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 Style/ClassStructure cop #5065

Merged
merged 4 commits into from Nov 23, 2017

Conversation

Projects
None yet
5 participants
@jonatas
Contributor

jonatas commented Nov 16, 2017

Following the Module/Class Structure.

It's a configurable cop that you can classify method macros and group them in the class. Example:

Style/ClassStructure:
  Enabled: true
  Categories:
    macro:
      - validates
      - validate
    include:
      - prepend
    attribute_macros:
      - attr_accessor
      - attr_reader
      - attr_writer
  ExpectedOrder:
      - extend
      - include
      - inner_class
      - constant
      - attribute_macros
      - macro
      - public_class_method
      - initialize
      - instance_method
      - protected_method
      - private_method

The Categories allows you to classify and group macros using the configuration.
The ExpectedOrder follows all macros and classification and complain when detecting something out of order.

It also autocorrects swapping the elements until it's totally organized.

Only elements that are in the ExpectedOrder configuration will be observed. If you don't care about where constants are placed, just remove from constant from the configuration.

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests(rake spec) are passing.
  • The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@jonatas jonatas changed the title from Add style class structure cop to Add Style/ClassStructure cop Nov 16, 2017

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Nov 16, 2017

Collaborator

This is some crazy (in a good sense 🙂) work right here. I'll start reviewing. We should probably add more thorough (granular) test coverage for added confidence.

I think it's a good decision to make it disabled by default. It requires some deliberation on the end of the
user to make proper use of this cop. 👍

Collaborator

Drenmi commented Nov 16, 2017

This is some crazy (in a good sense 🙂) work right here. I'll start reviewing. We should probably add more thorough (granular) test coverage for added confidence.

I think it's a good decision to make it disabled by default. It requires some deliberation on the end of the
user to make proper use of this cop. 👍

@pirj

pirj approved these changes Nov 16, 2017

Nothing to add! LGTM

Show outdated Hide outdated lib/rubocop/cop/style/class_structure.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
@pirj

pirj approved these changes Nov 19, 2017

Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
Show outdated Hide outdated spec/rubocop/cop/style/class_structure_spec.rb Outdated
@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Nov 20, 2017

Collaborator

Some thoughts along the way:

  1. This should go in Layout department, not Style.
  2. This needs to handle attribute macros that appear in private and protected scopes.
  3. We tend not to hard code Rails specific things into RuboCop.

🙂

Collaborator

Drenmi commented Nov 20, 2017

Some thoughts along the way:

  1. This should go in Layout department, not Style.
  2. This needs to handle attribute macros that appear in private and protected scopes.
  3. We tend not to hard code Rails specific things into RuboCop.

🙂

@jonatas

This comment has been minimized.

Show comment
Hide comment
@jonatas

jonatas Nov 20, 2017

Contributor

Thanks for the feedback @Drenmi.

  1. I was inspired by initial work of @alexdowad here.

  2. I'll need to figure out how to handle the macros in those different scopes. I was fighting exactly on this point and sometimes people want to use like attr_reader but also a private one. Not sure how to handle such situations.

  3. Yes. That is true. I was thinking about a way we can expand the configuration and allow to set specific rules for specific folders or class inheritance. Example:

CustomOrder:
  ApplicationRecord:
     - includes
     - constants
     - associations
     - validations
     - etc
  ApplicationJob:
     - constants
     - queue_priority
     - perform
     - private_methods
Contributor

jonatas commented Nov 20, 2017

Thanks for the feedback @Drenmi.

  1. I was inspired by initial work of @alexdowad here.

  2. I'll need to figure out how to handle the macros in those different scopes. I was fighting exactly on this point and sometimes people want to use like attr_reader but also a private one. Not sure how to handle such situations.

  3. Yes. That is true. I was thinking about a way we can expand the configuration and allow to set specific rules for specific folders or class inheritance. Example:

CustomOrder:
  ApplicationRecord:
     - includes
     - constants
     - associations
     - validations
     - etc
  ApplicationJob:
     - constants
     - queue_priority
     - perform
     - private_methods
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 21, 2017

Collaborator

Yes. That is true. I was thinking about a way we can expand the configuration and allow to set specific rules for specific folders or class inheritance. Example:

That seems like a good idea, but I'd start small. The main priority should be enforcing a consistent basic class structure, and everything else can be done afterwards. At this point I wouldn't bother much about which macro comes after which.

Collaborator

bbatsov commented Nov 21, 2017

Yes. That is true. I was thinking about a way we can expand the configuration and allow to set specific rules for specific folders or class inheritance. Example:

That seems like a good idea, but I'd start small. The main priority should be enforcing a consistent basic class structure, and everything else can be done afterwards. At this point I wouldn't bother much about which macro comes after which.

@jonatas

This comment has been minimized.

Show comment
Hide comment
@jonatas

jonatas Nov 21, 2017

Contributor

Great @bbatsov. So, are you ok with the simplest configuration I set? then I'll proceed autocorrecting the current issues and let's :shipit: 😄

Contributor

jonatas commented Nov 21, 2017

Great @bbatsov. So, are you ok with the simplest configuration I set? then I'll proceed autocorrecting the current issues and let's :shipit: 😄

@jonatas

This comment has been minimized.

Show comment
Hide comment
@jonatas

jonatas Nov 21, 2017

Contributor

I removed the rules for attribute_macros since some of them can be used in multiple visibilities.

Contributor

jonatas commented Nov 21, 2017

I removed the rules for attribute_macros since some of them can be used in multiple visibilities.

Show outdated Hide outdated CHANGELOG.md Outdated
Show outdated Hide outdated .rubocop.yml Outdated
Show outdated Hide outdated .rubocop.yml Outdated
Show outdated Hide outdated .rubocop.yml Outdated
Show outdated Hide outdated .rubocop.yml Outdated
Show outdated Hide outdated config/default.yml Outdated
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 22, 2017

Collaborator

Yeah, we can tackle the macros subsequently.

Collaborator

bbatsov commented Nov 22, 2017

Yeah, we can tackle the macros subsequently.

Show outdated Hide outdated config/default.yml Outdated

@bbatsov bbatsov merged commit d500d95 into rubocop-hq:master Nov 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 23, 2017

Collaborator

👍 🎉

Collaborator

bbatsov commented Nov 23, 2017

👍 🎉

@jonatas jonatas deleted the jonatas:add-style-class-structure-cop branch Nov 24, 2017

@jonatas

This comment has been minimized.

Show comment
Hide comment
@jonatas

jonatas Nov 30, 2017

Contributor

I created a blog post to show how to expand the configuration for other macros. Not sure if it worth to link it in somewhere:
http://ideia.me/customize-rubocop-layout-class-structure-cop

Contributor

jonatas commented Nov 30, 2017

I created a blog post to show how to expand the configuration for other macros. Not sure if it worth to link it in somewhere:
http://ideia.me/customize-rubocop-layout-class-structure-cop

@rickpeyton rickpeyton referenced this pull request Jan 11, 2018

Merged

Bump rubocop to latest #38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment