(PDOC-37) Warn when documented name does not match declared name #31

iankronquist · 2015-06-24T21:26:48Z

A rude first attempt. Let's see how this goes.

For warnings we just print to stderr. Note that Yard does not print its warnings to stderr, which is a bit crude.

Note that this does not do any type checking. Presently, if I understand correctly, our handlers do not add any type information so we can't compare types with the comments.
Addresses PDOC-37

HAIL9000 · 2015-06-24T23:39:46Z

lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb

      end
    end
-
+    #puts "params #{parameters} tags_hash #{tags_hash}"
+    #require 'pry'; binding.pry


Don't forget to remove your commented out debug statements ;)

Oops, sorry. Fixed.

iankronquist · 2015-06-25T17:14:21Z

What are peoples thoughts on disabling the Rubocop errors Metrics/AbcSize and Metrics/PerceivedComplexity? I'm all for linters making my code pretty, but these are just rubocop guessing at how complex the code is. I think it's mainly grouchy about all those ternary operators.

In any case, this PR does not introduce new linting errors.

iankronquist · 2015-06-25T17:25:35Z

So apparently rubocop doesn't check templates. I called it on this file explicitly. Never mind.

HAIL9000 · 2015-06-25T17:43:08Z

@iankronquist I think we should squash the most recent three commits (removing debug statements, updating warning messages, and fixing rubocop errors).

You can either squash them into your first commit, or sometimes what we do is have a commit for addressing pull request feedback where you can just describe all the things you changed.

But ultimately nothing in those commits is very interesting on its own, so it will make the git history a little easier for people to navigate in the future if we squash those.

hlindberg · 2015-06-25T20:21:02Z

We should have the same standard on RuboCop as we have for puppet IMO.

iankronquist · 2015-06-25T20:24:27Z

Squashed together.

HAIL9000 · 2015-06-25T21:06:28Z

@hlindberg We are using the same rubocop standards as puppet for strings. There was just some confusion because we don't use rubocop to check anything in the templates directory due to the fact that it's full of .erb files. :)

HAIL9000 · 2015-06-25T21:23:36Z

Okay, so I few suggestions. First, when I test I don't see file or line numbers, I see warnings like this:

[warning] Unknown file name or line number: The parameter num_c is documented, but doesn't exist in your code.

I also think that for consistency, we should try to make the format of our warnings similar to the way YARD does things, the same warning from YARD looks like this:

[warn]: @param tag has unknown parameter name: num_c
    in file `lib/puppet/functions/4x_function.rb' near line 12`

The last thing that I think would be ideal (unless this proves to be way too complicated and involve a lot of monkey patching) is to figure out how to silence YARD's warnings about missing parameters. Due to the fact that puppet functions are just a little different from Ruby, YARD will issue warnings for @param tags without existing parameters, even with the parameters are there. That means that our warnings are not particularly meaningful because they are mixed in with all the red herrings.

For example with this puppet function:

 # When given two numbers, returns the one that is larger.
 # You could have a several line description here if you wanted,
 # but I don't have much to say about this function.
 #
 # @example using two integers
 #   $bigger_int = max(int_one, int_two)
 #
 # @return [Integer] the larger of the two parameters
 #
 # @param num_c [Integer] the first number to be compared
 # @param num_b [Integer] the second number to be compared
 Puppet::Functions.create_function(:max) do
   def max(num_a, num_b)
     num_a >= num_b ? num_a : num_b
   end
 end

I see these warnings:

[warn]: @param tag has unknown parameter name: num_c
    in file `lib/puppet/functions/4x_function.rb' near line 12
[warn]: @param tag has unknown parameter name: num_b
    in file `lib/puppet/functions/4x_function.rb' near line 12
[warn]: @param tag has unknown parameter name: num_c
    in file `lib/puppet/functions/4x_function.rb' near line 12
[warning] Unknown file name or line number: The parameter num_c is documented, but doesn't exist in your code.

iankronquist · 2015-06-26T00:23:33Z

Fixed. I originally based the warning method off of clang because I think clang's warnings are pretty. This was probably unwise.
This turned out to be a bug. < should have been >=.
There is no easy way to fix this without monkey patching yard's docstring_transformer or registering our own tag, say puppet_parameter.

hlindberg · 2015-06-26T00:32:24Z

reg 3. Can we perhaps fool yard into believing there is such a parameter?

hlindberg · 2015-06-29T21:11:36Z

Not sure we can completely monkey patch away the yard behavior as it is of value when operating on Ruby code that is included in a module - helper classes etc. Is there some way we can make that conditional depending on if it is one of "our things" being processed or not?

HAIL9000 · 2015-06-29T21:24:20Z

@hlindberg That feels kind of tricky because puppet functions (which is the issue in question at the moment) is Ruby code which is being processed via the Ruby parser.

We do have our own handlers for puppet functions which inherit from YARD::Handlers::Ruby::Base, it may be possible to use those as a distinction somehow? I'm not sure what kind of information we have when we're in YARD::DocstringParser. If we have some information about the code object, we should be able to figure out if it is "ours" or not.

@iankronquist What do you think? Did you see any info about code objects when you were in there with the debugger?

iankronquist · 2015-07-02T17:01:57Z

Now with more tests! @hkenney @hlindberg r?

HAIL9000 · 2015-07-02T18:23:49Z

I'm working on trying to figure out how to update strings tests so that they are not tied to rspec 2. That way existing tests and the tests in this PR can coexist. Stay tuned

hlindberg · 2015-07-02T21:23:27Z

lib/puppet/face/strings.rb

@@ -50,6 +50,7 @@ def check_required_features
      # For now, assume the remaining positional args are a list of manifest
      # and ruby files to parse.
      yard_args = (args.empty? ? MODULE_SOURCEFILES : args)
+      class YARD::Logger; def progress(*args); end; end


break this up on multiple lines

Actually this is for debugging purposes and should be removed entirely. It's gutting a method so that threads do not get in the way of debugging.

ah ok - then it is better to have it as a one-liner :-) if useful later - comment it out and have a comment # For debugging, uncomment to .... or somesuch? I have used that in the parser in a place where it was difficult to remember where to add it back in when doing certain kinds of debugging. - just a though.

👍 Seems like a good idea to me! I use this line often (I have it written on a sticky note)

@iankronquist Could you go ahead and make this change? Just comment it out and leave a comment above explaining why it is useful for debugging (e.g. prevent threading so we can use the debugger)

Print the name of the parameter, the file, and the line number to stderr if the parameter name does not match the name specified in the docs. In order for this to be useful we need to present the user with the file name and line number of the relevant parameters. This information needs to be extracted from the code object and passed to the extract_param_details method.

Traverse the ruby AST to get method arguments and attach them to the code object. * Fixes spurious warnings issued by yard. * Obviates the need for ugly regex code in the templates to retrieve parameters from the source code.

Also comment out a statement which we frequently use for debugging which snuck in by accident.

hlindberg · 2015-07-06T17:38:59Z

+1 on merging

Override Yard logger on a per test basis, not globally. Replace one hack with another slightly less disgusting one.

(PDOC-37) Warn when documented name does not match declared name

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch 2 times, most recently from b96d7e0 to ff21c44 Compare June 24, 2015 22:02

iankronquist changed the title ~~(PDOC-21) Warn when documented type does not match declared type~~ (PDOC-37) Warn when documented name does not match declared name Jun 24, 2015

HAIL9000 reviewed Jun 24, 2015
View reviewed changes

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch from a25f157 to 0dca812 Compare June 25, 2015 18:34

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch from 0dca812 to 0969c63 Compare June 25, 2015 20:24

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch from 0969c63 to 14701f4 Compare June 26, 2015 00:22

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch from 5e163c5 to 565b9ae Compare June 29, 2015 20:40

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch 3 times, most recently from 306c88d to aa8b95a Compare July 2, 2015 16:59

hlindberg reviewed Jul 2, 2015
View reviewed changes

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch from aa8b95a to b377424 Compare July 6, 2015 17:14

iankronquist added 2 commits July 6, 2015 10:18

(PDOC-37) Attach params to Puppet 4.x code object

463d4e0

Traverse the ruby AST to get method arguments and attach them to the code object. * Fixes spurious warnings issued by yard. * Obviates the need for ugly regex code in the templates to retrieve parameters from the source code.

(PDOC-37) Test errors are output properly

d37e071

Also comment out a statement which we frequently use for debugging which snuck in by accident.

iankronquist force-pushed the warn-when-not-matching/PDOC-21 branch from b377424 to d37e071 Compare July 6, 2015 17:19

(PDOC-37) Override Yard logger on a per test basis

168c1c4

Override Yard logger on a per test basis, not globally. Replace one hack with another slightly less disgusting one.

HAIL9000 added a commit that referenced this pull request Jul 7, 2015

Merge pull request #31 from iankronquist/warn-when-not-matching/PDOC-21

c8fbe3c

(PDOC-37) Warn when documented name does not match declared name

HAIL9000 merged commit c8fbe3c into puppetlabs:master Jul 7, 2015

chelnak added the bugfix label Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDOC-37) Warn when documented name does not match declared name #31

(PDOC-37) Warn when documented name does not match declared name #31

iankronquist commented Jun 24, 2015

HAIL9000 Jun 24, 2015

iankronquist Jun 25, 2015

iankronquist commented Jun 25, 2015

iankronquist commented Jun 25, 2015

HAIL9000 commented Jun 25, 2015

hlindberg commented Jun 25, 2015

iankronquist commented Jun 25, 2015

HAIL9000 commented Jun 25, 2015

HAIL9000 commented Jun 25, 2015

iankronquist commented Jun 26, 2015

hlindberg commented Jun 26, 2015

hlindberg commented Jun 29, 2015

HAIL9000 commented Jun 29, 2015

iankronquist commented Jul 2, 2015

HAIL9000 commented Jul 2, 2015

hlindberg Jul 2, 2015

HAIL9000 Jul 2, 2015

hlindberg Jul 2, 2015

HAIL9000 Jul 2, 2015

HAIL9000 Jul 6, 2015

iankronquist Jul 6, 2015

hlindberg commented Jul 6, 2015

(PDOC-37) Warn when documented name does not match declared name #31

(PDOC-37) Warn when documented name does not match declared name #31

Conversation

iankronquist commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iankronquist commented Jun 25, 2015

iankronquist commented Jun 25, 2015

HAIL9000 commented Jun 25, 2015

hlindberg commented Jun 25, 2015

iankronquist commented Jun 25, 2015

HAIL9000 commented Jun 25, 2015

HAIL9000 commented Jun 25, 2015

iankronquist commented Jun 26, 2015

hlindberg commented Jun 26, 2015

hlindberg commented Jun 29, 2015

HAIL9000 commented Jun 29, 2015

iankronquist commented Jul 2, 2015

HAIL9000 commented Jul 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlindberg commented Jul 6, 2015