Skip to content

Enable Rubocop accessor grouping, fix existing offenses#8293

Merged
jekyllbot merged 8 commits intojekyll:masterfrom
torrocus:refactor/enable-accessor-grouping
Jul 23, 2021
Merged

Enable Rubocop accessor grouping, fix existing offenses#8293
jekyllbot merged 8 commits intojekyll:masterfrom
torrocus:refactor/enable-accessor-grouping

Conversation

@torrocus
Copy link
Copy Markdown
Contributor

@torrocus torrocus commented Jul 9, 2020

This is refactoring.

Summary

Enables Rubocop Style/AccessorGrouping rule.
And fixes existing offenses related to accessor grouping.
Accessors with comments still have them, but in some cases I shortened them to the essential context.

Context

It isn't related to any GitHub issue.
The goal of this PR is to to use more strict rules with Rubocop.
Consequently, this is to ensure greater readability and uniformity of the code.
The functionality is the same.

@torrocus torrocus changed the title Enable Rubocop accessor grouping, fix existing offenses [WIP] Enable Rubocop accessor grouping, fix existing offenses Jul 9, 2020
@torrocus torrocus force-pushed the refactor/enable-accessor-grouping branch from db75644 to b8bfc8d Compare July 9, 2020 07:45
@torrocus torrocus changed the title [WIP] Enable Rubocop accessor grouping, fix existing offenses Enable Rubocop accessor grouping, fix existing offenses Jul 9, 2020
Copy link
Copy Markdown
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@DirtyF DirtyF requested a review from ashmaroli July 9, 2020 09:15
@DirtyF DirtyF added the internal label Jul 9, 2020
Copy link
Copy Markdown
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

On the whole, I'm not a fan of listing attributes vertically in the alphabetic order because:

  • longer lists pushes rest of the class structure downwards — more scrolling!
  • when a new attribute is added, there's an (unnecessary) extra overhead to insert it in the correct place on the list for consistency.

Finding a balance between the above two is hard..

@torrocus
Copy link
Copy Markdown
Contributor Author

torrocus commented Jul 9, 2020

@ashmaroli
I will present a different point of view. Maybe it will convince you.

Alphabetical order means that we subconsciously know where to look for a given attribute.
The alphabet has been with us since childhood and is everywhere
(ex. phone book, dictionary, list of countries, cities, languages on the forms).
Our brain loves patterns, and the alphabet is one of them.

I understand that longer lists pushes rest of the class structure downwards.
But this is not a problem with attributes, but a problem with the whole class.
Personally, I think classes should be small.
And most importantly they should do one thing at a time (SOLID principles).

If each attribute is on a separate line, adding or removing attributes changes only 1 line.
This is a big advantage, because the git history is preserved for the other attributes.

In addition, most editors have the ability to sort multiple lines, so keeping order is easy.
What editor are you using?

DirtyF and others added 7 commits May 18, 2021 09:38
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
@DirtyF
Copy link
Copy Markdown
Member

DirtyF commented Jul 23, 2021

@jekyll: merge +dev

@jekyllbot jekyllbot merged commit 94fcfdd into jekyll:master Jul 23, 2021
@jekyllbot jekyllbot added the fix label Jul 23, 2021
jekyllbot added a commit that referenced this pull request Jul 23, 2021
github-actions bot pushed a commit that referenced this pull request Jul 23, 2021
Alex Malaszkiewicz: Enable Rubocop accessor grouping, fix existing offenses (#8293)

Merge pull request 8293
@jekyll jekyll locked and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants