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

[Fix #6931] Layout/AccessModifierIndentation outdent style now ignores modifiers with parameters #6935

Merged
merged 6 commits into from Apr 22, 2019
Merged

[Fix #6931] Layout/AccessModifierIndentation outdent style now ignores modifiers with parameters #6935

merged 6 commits into from Apr 22, 2019

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Apr 15, 2019

This is the fix I propose for #6931.

It doesn't include the remane of Layout/AccessModifierIndentation to Layout/BareAccessModifierIndentation though. I can add that if needed.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • 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.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@deivid-rodriguez deivid-rodriguez changed the title [Fix #6931] Access modifier outdent ignores modifiers with parameters [Fix #6931] Layout/AccessModifierIndentation outdent style now ignores modifiers with parameters Apr 15, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 17, 2019

The change looks good to me. Can you confirm it's consistent with how they do things in Rails?

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 17, 2019

One more thing - you can also add some test cases of a modifier being used directly with def.

@deivid-rodriguez
Copy link
Contributor Author

Thanks for having a look @bbatsov!

Rails doesn't use this cop. They do have Style/IndentationConsistency enabled with EnforcedStyle: Rails style, which still passes after my changes. Is that what you wanted me to check?

Regarding the new tests, I'll add them.

Should I also rename the cop?

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 21, 2019

Should I also rename the cop?

No, I'd rather just update the cop documentation to be more explicit about its scope and what's outside of it.

@deivid-rodriguez
Copy link
Contributor Author

I tried to clarify the docs a bit, and added a changelog entry. I tagged it as a bug fix. The alternative would be to tag this as a change and change the VersionChanged attribute in the cop, I guess?

@bbatsov bbatsov merged commit 48e63dd into rubocop:master Apr 22, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 22, 2019

It's fine as it is. Thanks!

@deivid-rodriguez
Copy link
Contributor Author

Great, thanks!

ghost pushed a commit to rubygems/bundler that referenced this pull request Apr 30, 2019
7151: Bump rubocop to 0.68.0 r=hsbt a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was that our rubocop version is getting outdated. Also, I added [a PR to rubocop](rubocop/rubocop#6935) so that we can remove the exclusions in our configuration, and it has now been released.

### What is your fix for the problem, implemented in this PR?

My fix is to update rubocop to the latest version, update the configuration, and fix new offenses.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
hsbt pushed a commit to rubygems/bundler-graph that referenced this pull request Oct 19, 2021
7151: Bump rubocop to 0.68.0 r=hsbt a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was that our rubocop version is getting outdated. Also, I added [a PR to rubocop](rubocop/rubocop#6935) so that we can remove the exclusions in our configuration, and it has now been released.

### What is your fix for the problem, implemented in this PR?

My fix is to update rubocop to the latest version, update the configuration, and fix new offenses.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
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

2 participants