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

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

Merged
merged 5 commits into from
Oct 24, 2019

Conversation

redsummernight
Copy link
Member

Issue

https://otwarchive.atlassian.net/browse/AO3-5758

Purpose

Turn off some checks that we always ignore.

Testing Instructions

None, it's just Hound.

@redsummernight redsummernight added Awaiting Review Scope: Tests Only Only changes automated tests or test configuration labels Sep 21, 2019
Layout/DotPosition:
EnforcedStyle: leading

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:

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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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: 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
])

@sarken sarken merged commit ff7f628 into otwcode:master Oct 24, 2019
@redsummernight redsummernight deleted the AO3-5758-hound branch October 24, 2019 00:25
sarken pushed a commit to sarken/otwarchive that referenced this pull request Oct 24, 2019
* AO3-5758 Add Hound rules for bundler, layout, and style

* Add Layout/IndentArray rule

* Use aligned + trailing for multiline method calls

* Use indented + leading for multiline method calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed: Ready to Merge Scope: Tests Only Only changes automated tests or test configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants