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 #8431] Add Safety section to documentation for all cops that are Safe: false or SafeAutoCorrect: false #10094

Merged
merged 2 commits into from Sep 20, 2021

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Sep 16, 2021

Documents safety concerns for every cop that is either Safe: false or SafeAutoCorrect: false. This documentation will show up as a separate subsection in each cop's documentation once the .adoc files are generated.

For example:
image

In order to make this happen, I added a custom @safety YARD tag that is used to process the documentation, which should make it simple when creating a new unsafe cop (I also updated the rake new_cop template) or changing a cop to be unsafe.

The text for the safety section comes either from the text that already existed in the cop (🙌 to everyone who added a reason when marking a cop unsafe); looking through many old PRs in order to try to determine why the cop was made unsafe; or using my best guess if I could not find a better reason. While I was going through documentation I took the opportunity to make some minor tweaks as well.

I tried writing a project_spec test to ensure a @safety block is added for cops that are unsafe, but could not find a way to get the yard code object for a class (the documentation generator relies on yard's generation rake task, which I did not want to run in a spec). If anyone has any suggestions please let me know!

Altogether, 66 cops were updated by this change.

Fixes #8431.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

Register a `@safety` custom YARD tag, and uses it to create a new section to the documentation about cop safety. Also updates the `new_cop` template to add the `@safety` tag section.
Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

Fantastic work! This looks very user friendly! IMO, it's better to have a project_spec for this, but this PR changes themself are well worth!

@dvandersluis
Copy link
Member Author

dvandersluis commented Sep 17, 2021

IMO, it's better to have a project_spec for this

So as far as I can tell, we need YARD to have regenerated the .yardoc folder with the latest changes, and then we can load it in order to check the project. This should work for rake default because it calls documentation_syntax_check which calls yard_for_generate_documentation. It should be reasonably fast too.

However, if rake spec is run without the yardoc being generated first, tests will fail. I'm not sure if we should do this as a separate test run as part of rake default? Or somehow exclude it when running rake spec? And then we'd have to figure out how to handle it in CI as well.

If we can solve these, here is a scaffold for a test: https://gist.github.com/dvandersluis/0d692eafd4696f542f5bd6abd1840043. We can conceivably extend this to check for missing @example tags too, etc. (might be worthwhile to do all this as a separate PR, once we have decided on a solution).

@koic
Copy link
Member

koic commented Sep 17, 2021

IMO, it's better to have a project_spec for this

Thank you for investigating. Unfortunately I'm not sure about it right away 💦 (and so far I don't have a strong opinion yet.)

might be worthwhile to do all this as a separate PR, once we have decided on a solution

Yeah, I think we can add that spec later when we are really in trouble. First of all, I think the content of the current PR is sufficient 😃

@dvandersluis
Copy link
Member Author

Sounds good to me!

Copy link
Collaborator

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

Fantastic change!

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 20, 2021

If we can solve these, here is a scaffold for a test: https://gist.github.com/dvandersluis/0d692eafd4696f542f5bd6abd1840043. We can conceivably extend this to check for missing @example tags too, etc. (might be worthwhile to do all this as a separate PR, once we have decided on a solution).

I agreed. I've long wanted to automate checking the docs for some required structure and it'd be great to eventually get there. Step by step... :-)

@bbatsov bbatsov merged commit 28ae02a into rubocop:master Sep 20, 2021
@dvandersluis dvandersluis deleted the issue/8431 branch September 27, 2021 11:17
koic added a commit to rubocop/rubocop-minitest that referenced this pull request Oct 4, 2021
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 10, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 11, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 11, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 11, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 11, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 11, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 11, 2021
Follow up to Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 11, 2021
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 11, 2021
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 12, 2021
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 12, 2021
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 12, 2021
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are
`Safe: false` or `SafeAutoCorrect: false`.
renawatson68 added a commit to renawatson68/performance-develop-rubyonrails that referenced this pull request Sep 23, 2022
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
richardstewart0213 added a commit to richardstewart0213/performance-build-Performance-optimization-analysis- that referenced this pull request Nov 4, 2022
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
MarttiCheng added a commit to MarttiCheng/Rubocop-Performance that referenced this pull request Sep 28, 2023
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
SerhiiMisiura added a commit to SerhiiMisiura/Rubocop-Performance that referenced this pull request Oct 5, 2023
Follow up to rubocop/rubocop#10094.

This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document safety issues
3 participants