Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DocstringParser warnings improvements #1488

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

notEthan
Copy link
Contributor

@notEthan notEthan commented Mar 7, 2023

Several improvements:

  • Warnings about unknown params are skipped when the param is delegated to another method, determined by the param tag being a reference and the method taking a splat argument.
  • param reference tags' names are used when possible.
  • DocstringParser warnings include parser.object in the message - having the file/line number as it does now is sufficient, but jumping to the method name or class name can be simpler

The first one was what motivated this work, I wanted to get rid of warnings about delegated params. The rest were changes I made along the way.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@notEthan notEthan force-pushed the docstring_parser_param_warnings branch from e2bf8e5 to 8ae1ea7 Compare March 7, 2023 20:30
@notEthan
Copy link
Contributor Author

notEthan commented Mar 11, 2023

This is incomplete, it should handle argument forwarding with def foo(...) as well as splat. currently YARD::Handlers::Ruby::MethodHandler does not add a parameter for a ... param, I'm having a look at what might be done with that. I could add a param named ... there (haven't explored what else that might affect), but I am thinking maybe better to just drop the check for a splat/forwarding param here, not warn about unknown params that are (see ...) refs at all, trusting that they are handled.

Copy link

@Skipants Skipants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope you don't mind some random reviewing your PR. I'm using Yard a lot lately and am invested in helping out.

else
param_tags << tag
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can flatten down this if and improve readability IMO by using .filter_map to iterate here. Consider:

param_tags = parser.tags.filter_map do |tag|
  next unless tag.tag_name == "param"
  next tag if !tag.is_a?(Tags::RefTagList) || tag.name
  
  tag.tags unless CodeObjects::Proxy === tag.owner
end

filter_map is Ruby 2.7+ and I'm not sure about Yard's current minimum Ruby version, so if it's lower you can use map{ ... }.compact instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants