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.

Sign up for GitHub

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

Jump to bottom

AO3-5758 Add Hound rules for bundler, layout, and style #3641

Merged
merged 5 commits into from
Oct 24, 2019
Merged

AO3-5758 Add Hound rules for bundler, layout, and style #3641

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42c730a
AO3-5758 Add Hound rules for bundler, layout, and style
redsummernight Sep 21, 2019
b8de976
Add Layout/IndentArray rule
redsummernight Sep 21, 2019
2ae9728
Use aligned + trailing for multiline method calls
redsummernight Oct 5, 2019
11a1628
Use indented + leading for multiline method calls
redsummernight Oct 21, 2019
98dcfc8
Merge branch 'master' into AO3-5758-hound
redsummernight Oct 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions config/.rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,18 @@
AllCops:
TargetRubyVersion: 2.6

Bundler/OrderedGems:
Enabled: false

Layout/DotPosition:
EnforcedStyle: leading

Layout/IndentArray:
EnforcedStyle: consistent
Copy link
Member Author

Choose a reason for hiding this comment

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

# consistent
array = [
  :value
]
and_in_a_method_call([
  :no_difference
])


Layout/MultilineMethodCallIndentation:
Copy link
Member Author

Choose a reason for hiding this comment

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

With these 2 layout rules, Hound will ask us to rewrite, for example:

otwarchive/app/controllers/api/v2/bookmarks_controller.rb

Lines 64 to 66 in 67b7b14

archivist_bookmarks = archivist_bookmarks
.select { |b| b&.bookmarkable.is_a?(ExternalWork) ? b&.bookmarkable&.url == current_bookmark_url : false }
.map { |b| [b, b.bookmarkable] }

into:

archivist_bookmarks = archivist_bookmarks
  .select { |b| b&.bookmarkable.is_a?(ExternalWork) ? b&.bookmarkable&.url == current_bookmark_url : false }
  .map    { |b| [b, b.bookmarkable] }

Copy link
Member

Choose a reason for hiding this comment

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

Should we do this? I think we use the first style more than the second.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can switch to indented_relative_to_receiver.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I really don't know what the thing to do here is! I do see a benefit to using what you've put here -- less indenting means more room for your line of code, which is nice. However, I was under the impression we've mainly used the first approach over the years, which would mean getting a lot more Hound complaints as we edit stuff.

So I really am asking, does my perception of what we've been doing align with others', and if so, should we make this change anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the leading indentation, but I've also always been a trailing dot person - when we started the project, we were still on Ruby 1.8, which didn't even allow the leading dot syntax. So there's got to be a lot of that in the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Easier copying and pasting to the console is a good point and makes me lean even more strongly toward sticking with trailing, as does the note about comments in rubocop/ruby-style-guide#176 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

...however, upon further thought, if we switch our indenting to plain old indented, I will be 100% in favor of leading-dot. It does make a huge difference in being able to understand wtf is going on in that case.

archivist_bookmarks = archivist_bookmarks
  .select { |b| b&.bookmarkable.is_a?(ExternalWork) ? b&.bookmarkable&.url == current_bookmark_url : false }
  .map    { |b| [b, b.bookmarkable] }

vs

archivist_bookmarks = archivist_bookmarks.
  select { |b| b&.bookmarkable.is_a?(ExternalWork) ? b&.bookmarkable&.url == current_bookmark_url : false }.
  map    { |b| [b, b.bookmarkable] }

But if we use aligned or indented_relative_to_receiver, I think we should keep our dots trailing. The deep indenting is a pretty clear signal that the second line is a chained method call.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we have 2 options:

  1. indented + leading
  2. aligned/indented_relative_to_receiver + trailing

I like indented because the later lines won't have to be re-indented any time the first line changes, less diffs. I'd prefer 1.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer 1 as well.

Copy link
Member

Choose a reason for hiding this comment

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

Let's 🐹 this at 1, then!

EnforcedStyle: indented

Layout/TrailingWhitespace:
Enabled: false

Expand All @@ -28,10 +40,21 @@ Metrics/ModuleLength:
Style/FormatStringToken:
EnforcedStyle: template

Style/FrozenStringLiteralComment:
Enabled: false

Style/GlobalVars:
AllowedVariables:
- $elasticsearch
- $rollout

# stop checking if uses of "self" are redundant
Style/RedundantSelf:
Enabled: false

# stop checking quotation marks
Style/StringLiterals:
Enabled: false

Style/SymbolArray:
Enabled: false