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

Add new Style/MagicCommentFormat cop #9364

Conversation

mattbearman
Copy link
Contributor

@mattbearman mattbearman commented Jan 11, 2021

Magic comments in Ruby are underscore/hyphen agnostic, this can lead to codebases having inconsistent case style in the frozen string literal comment (# frozen_string_literal: true and # frozen-string-literal: true)

This cop allows the enforcement of either snake case (underscore separated) or kebab case (hyphen separated).


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.

@mattbearman mattbearman force-pushed the frozen-string-literal-comment-case-style-cop branch 2 times, most recently from 1020f33 to 953c26a Compare January 13, 2021 10:12
@mattbearman mattbearman changed the title Frozen string literal comment case style cop Add frozen string literal comment case style cop Jan 13, 2021
@mattbearman mattbearman force-pushed the frozen-string-literal-comment-case-style-cop branch 2 times, most recently from 7312063 to 4b2752c Compare January 13, 2021 14:21
@mattbearman mattbearman marked this pull request as ready for review January 13, 2021 14:36
@mattbearman mattbearman changed the title Add frozen string literal comment case style cop Add new Style/FrozenStringLiteralCommentCaseStyle cop Jan 13, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 15, 2021

I think it'd better is this was a cop covering all such pragmas. One cop per pragma seems like a massive overkill to me.

@mattbearman
Copy link
Contributor Author

That's a great point, I'll see about extending this to all magic comments 👍

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 12, 2021

@mattbearman ping :-)

@bbatsov bbatsov self-assigned this Feb 15, 2021
@dvandersluis
Copy link
Member

@mattbearman are you still interested in completing this?

@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from 4b2752c to f100e51 Compare August 9, 2022 16:59
@dvandersluis dvandersluis changed the title Add new Style/FrozenStringLiteralCommentCaseStyle cop Add new Style/MagicComments cop Aug 9, 2022
@dvandersluis
Copy link
Member

@bbatsov I have updated this PR to complete it.

The cop is now called Style/MagicComments, and it enforces consistency on all magic comments, not just frozen-string-literal. I also added the ability to keep capitalization consistent, for both the comment directives and values independently.

@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from f100e51 to 3679fc2 Compare August 9, 2022 17:17
@dvandersluis
Copy link
Member

Some tests are failing randomly and I haven't been able to repro yet. Looking into it.

@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from 2d3f19b to 6bd5948 Compare August 9, 2022 17:42
@dvandersluis
Copy link
Member

Ah, figured it out, filter_map isn't available in all rubies 😁 Fixed it now.

@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from 6bd5948 to 3a08351 Compare August 9, 2022 17:49
# in kebab case (words separated by hyphens).
# Eg: froze-string-literal: true
- kebab
DirectiveCapitalization: lower
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to me this can simply be a boolean.

Copy link
Member

Choose a reason for hiding this comment

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

There are three options - upper (all uppercase), lower (all lowercase) and nil (ignore capitalization)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I saw this, but was on the fence for whether it makes sense for someone to disable this. I've yet to see someone not writing lowercase magic comments, but I guess they exist. :D

Copy link
Member

Choose a reason for hiding this comment

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

I figured that since it's easy, having options is better 😄

@koic
Copy link
Member

koic commented Aug 10, 2022

Style/MagicComments may be abstract. A somewhat concreted name like Style/MagicCommentFormat, Style/MagicCommentShape, or more better name looks user friendly.

config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from 3a08351 to 0373857 Compare August 10, 2022 02:22
@dvandersluis
Copy link
Member

Style/MagicComments may be abstract. A somewhat concreted name like Style/MagicCommentFormat, Style/MagicCommentShape, or more better name looks user friendly.

Sure, MagicCommentFormat works for me.

@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from 0373857 to a2f19bb Compare August 10, 2022 02:31
@dvandersluis dvandersluis changed the title Add new Style/MagicComments cop Add new Style/MagicCommentFormat cop Aug 10, 2022
DirectiveCapitalization: lower
ValueCapitalization: ~
SupportedCapitalizations:
- lower
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I like DirectiveCase a bit better as capitalization and lowercase don't mix very well IMO. :-) Perhaps spelling out fully lowercase and uppercase would be best.

Copy link
Member

Choose a reason for hiding this comment

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

I had DirectiveCase first but I found it confusing with snake case and kebab case. I'll change it to the full lowercase and uppercase, good suggestion!

Copy link
Member

Choose a reason for hiding this comment

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

Updated!

@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from a2f19bb to 0f0c7b9 Compare August 10, 2022 04:53
…nsistent magic comments.

This cop is designed to ensure frozen string literal comments
are consistent, enforcing either snake case or kebab case, as well as capitalization.

Co-authored-by: Matt Bearman <matt@mattbearman.com>
@dvandersluis dvandersluis force-pushed the frozen-string-literal-comment-case-style-cop branch from 0f0c7b9 to a425518 Compare August 10, 2022 04:53
@bbatsov bbatsov merged commit 8ca7aae into rubocop:master Aug 10, 2022
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

4 participants