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

Create a new cop group, MagicComments #3886

Closed
wants to merge 1 commit into from

Conversation

rrosenblum
Copy link
Contributor

These cops always felt a bit weird being classified as Style cops. I think that this creates a good classification for grouping these cops.

This PR creates a new MagicComments group. It also moves Style/Encoding to MagicComments/Encoding and Style/FrozenStringLiteralComment to MagicComments/FrozenStringLiteral.

My vision for the future of this is to add a cop for warn_indent. Possibly add support for a warn_past_scope cop. Standardize the comment checks across all of these cops. Create a common configuration that determines whether the comments should all be on one line or all on separate lines. Create a configuration to set the order of how the comments should appear.

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 9, 2017

Thinking if this could be generalized to also include the cops for documentation comments?

@rrosenblum
Copy link
Contributor Author

@Drenmi can you clarify that a little bit. It sounds like you are suggesting to create a Comment or Comments cop group that will contain the magic comments cops, Style/DocumentationMethod, Style/Documentation, Style/BlockComments, Style/InlineComment, Style/AsciiComments, Style/CommentIndentation, and Style/CommentAnnotation.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 9, 2017

My vision for the future of this is to add a cop for warn_indent. Possibly add support for a warn_past_scope cop. Standardize the comment checks across all of these cops. Create a common configuration that determines whether the comments should all be on one line or all on separate lines. Create a configuration to set the order of how the comments should appear.

Sounds great to me.

@deivid-rodriguez
Copy link
Contributor

Not sure how to explain it, but this new group feels a bit off to me. I don't feel it fits into the Perfomance, Style, Metrics division....

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 10, 2017

can you clarify that a little bit. It sounds like you are suggesting to create a Comment or Comments cop group [...]

Actually my train of thought has not reached any station yet. I think I started in the same place as you, with: "this isn't really about coding style, is it?" 😅

@rrosenblum
Copy link
Contributor Author

I don't feel it fits into the Perfomance, Style, Metrics division....

Understandable. The same can be said about Bundler and Rails. MagicComments seemed like the best descriptor of this new group.

This conversation has caused me to think about the relationship that we have between cops and groups for all of the cops. Has anyone ever considered adding sub-categories to our groups? For example, what if we had Style/Alignment and this group contained what is currently AlignArray, AlignHash, AlignParameters, ElseAlignment, and RescueEnsureAlignment.

@backus
Copy link
Contributor

backus commented Jan 10, 2017

Has anyone ever considered adding sub-categories to our groups? For example, what if we had Style/Alignment and this group contained what is currently AlignArray, AlignHash, AlignParameters, ElseAlignment, and RescueEnsureAlignment.

Yeah I was really interested in this because it would make it way easier for 3rd party cops. See rubocop/rubocop-rspec#232

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 11, 2017

Has anyone ever considered adding sub-categories to our groups? For example, what if we had Style/Alignment and this group contained what is currently AlignArray, AlignHash, AlignParameters, ElseAlignment, and RescueEnsureAlignment.

I don't recall us ever discussing something like this. Might be makes to some extent. Or we can assume that Style is a super broad category... I don't know. :-)

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 4, 2017

Closing due to lack of progress here.

@bbatsov bbatsov closed this Mar 4, 2017
@rrosenblum
Copy link
Contributor Author

Closing due to lack of progress here.

I felt like there wasn't much that came out of the conversation on this. Is this something that we would want to work towards or should this be considered completely dead? If it is something that we would like to move towards, what kind of changes would you like to see?

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 6, 2017

I'm open to the idea of breaking down the Style namespace, but I'm not sure I want a separate namespace for magic comments. I'd love to see those 2 cops you mentioned in the PR, though.

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

5 participants