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

[Fix #6708] Style/CommentedKeyword knows :yields: #6735

Merged
merged 1 commit into from Feb 6, 2019

Conversation

bquorning
Copy link
Contributor

The Style/CommentedKeyword cop would not allow the :yields: RDoc directive as a trailing comment (to def). The documentation https://docs.ruby-lang.org/en/2.1.0/RDoc/Markup.html#class-RDoc::Markup-label-Directives lists many, many directives, but only :nodoc: and :yields: are shown as trailing comments.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -59,7 +59,7 @@ def investigate(processed_source)
private

KEYWORDS = %w[begin class def end module].freeze
ALLOWED_COMMENTS = %w[:nodoc: rubocop:disable].freeze
ALLOWED_COMMENTS = %w[:nodoc: :yields: rubocop:disable].freeze
Copy link
Member

Choose a reason for hiding this comment

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

This change also update the document.
https://github.com/rubocop-hq/rubocop/blob/v0.63.1/lib/rubocop/cop/style/commented_keyword.rb#L9

And it may be better to be able to customize with default.yml (.rubocop.yml) in the future :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The document says “such as :nodoc: and rubocop:disable”, which I thought means it doesn’t need to list all allowed comments. Would you prefer listing all allowed comments there? E.g.

# Note that some comments (`:nodoc:`, `:yields:, and `rubocop:disable`)
# are allowed.

I wouldn’t go for full customization (giving an array in default.yml) just yet. I’d be interested to see – when a bug is reported – which kind of comment would need to be added to the list.

Copy link
Member

Choose a reason for hiding this comment

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

Would you prefer listing all allowed comments there? E.g.

Yes. I think that's fine.

when a bug is reported – which kind of comment would need to be added to the list.

OK. Let's think about it at that time.

Copy link
Contributor Author

@bquorning bquorning Feb 6, 2019

Choose a reason for hiding this comment

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

I’ve pushed the new documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for quick updating!

The `Style/CommentedKeyword` cop would not allow the `:yields:` RDoc directive
as a trailing comment (to `def`). The documentation https://docs.ruby-lang.org/
en/2.1.0/RDoc/Markup.html#class-RDoc::Markup-label-Directives lists many, many
directives, but only `:nodoc:` and `:yields:` are shown as trailing comments.
@koic koic merged commit 2269afc into rubocop:master Feb 6, 2019
@bquorning bquorning deleted the allow-yields-rdoc-comment branch February 6, 2019 09:12
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.

None yet

2 participants