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

Rename enforced style rails for indentation consistency #7113

Conversation

Projects
None yet
3 participants
@koic
Copy link
Member

commented Jun 6, 2019

Rename EnforcedStyle: rails for Layout/IndentationConsistency

Follow up #7059 (comment).

This PR renames EnforcedStyle: rails to EnabledStyle: indented for Layout/IndentationConsistency. This makes it a generic name that doesn't depend on Rails.

And this PR shows alternative for obsolete EnforcesStyle parameter name.

For example, EnforcedStyle: rails in config/default.yml will be replaced by EnforcedStyle: indented.

 # .rubocop.yml
 Layout/IndentationConsistency:
-  EnforcedStyle: rails
+  EnforcedStyle: indented

Before

After EnforcedStyle: rails has been obsoleted, Users do not know what the parameter name to replace.

% rubocop
Error: invalid EnforcedStyle 'rails' for Layout/IndentationConsistency found in .rubocop.yml
Valid choices are: normal, indented

After

Users will find that they should use EnforcedStyle: indented instead.

% rubocop
Error: obsolete `EnforcedStyle: rails` (for Layout/IndentationConsistency)found in .rubocop.yml
`EnforcedStyle: rails` has been renamed to `EnforcedStyle: indented`

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.
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

I wonder if outdented_access_modifiers won't be a better name for this style, as indented doesn't mean much for something like indentation consistency. Any thoughts @rubocop-hq/rubocop-core?

@koic

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Thanks for proposing a better option name. I'd like to update it to outdented_access_modifiers if there is no other opinion.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

@koic That's fine by me.

@koic koic referenced this pull request Jun 11, 2019

Merged

Merge two Bug fixes entry into one #7126

4 of 8 tasks complete

koic added some commits Jun 6, 2019

Rename `EnforcedStyle: rails` for `Layout/IndentationConsistency`
Follow up
#7059 (comment).

This PR renames `EnforcedStyle: rails` to `EnabledStyle: outdented_access_modifiers`
for `Layout/IndentationConsistency`.
This makes it a generic name that doesn't depend on Rails.
Show alternative for obsolete EnforcesStyle parameter name
This PR shows alternative for obsolete EnforcesStyle parameter name.

For example, `EnforcedStyle: rails` in config/default.yml will be
replaced by `EnforcedStyle: outdented_access_modifiers`.

```diff
 # .rubocop.yml
 Layout/IndentationConsistency:
-  EnforcedStyle: rails
+  EnforcedStyle: outdented_access_modifiers
```

### Before

After `EnforcedStyle: rails` has been obsoleted, Users do not know
what the parameter name to replace.

```consle
% rubocop
Error: invalid EnforcedStyle 'rails' for Layout/IndentationConsistency
found in .rubocop.yml
Valid choices are: normal, outdented_access_modifiers
```

### After

Users will find that they should use `EnforcedStyle: outdented_access_modifiers` instead.

```console
% rubocop
Error: obsolete `EnforcedStyle: rails` (for Layout/IndentationConsistency)
found in .rubocop.yml
`EnforcedStyle: rails` has been renamed to `EnforcedStyle: outdented_access_modifiers`
```

@koic koic force-pushed the koic:rename_enforced_style_rails_for_indentation_consistency branch from 4f0480c to e37d1e5 Jun 12, 2019

@koic

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Alternative enforced style name updated from indented to outdented_access_modifiers.

@bbatsov bbatsov merged commit 1dc3248 into rubocop-hq:master Jun 12, 2019

21 of 22 checks passed

ci/circleci: ruby-head-rubocop Your tests failed on CircleCI
Details
ci/circleci: cc-setup Your tests passed on CircleCI!
Details
ci/circleci: cc-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: documentation-checks Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-spec Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@koic koic deleted the koic:rename_enforced_style_rails_for_indentation_consistency branch Jun 12, 2019

@kamipo

This comment has been minimized.

Copy link

commented Jun 12, 2019

Probably EnforcedStyle: rails was intended to be used in rails/rails, but we (rails/rails) basically do not want to be outdented access modifiers.

# not better

class Foo
  def bar
  end

private
  def baz
  end
end

# good

class Foo
  def bar
  end

  private
    def baz
    end
end

The naming outdented_access_modifiers cause misreading that rails/rails want to the above # not better style rather than # good style, it is a little unhappy to me.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

Probably EnforcedStyle: rails was intended to be used in rails/rails, but we (rails/rails) basically do not want to be outdented access modifiers.

Yeah, that was the case.

The naming outdented_access_modifiers cause misreading that rails/rails want to the above # not better style rather than # good style, it is a little unhappy to me.

I'm puzzled. Did the style in Rails change at some point? If so I guess the name of the style and its description should be changed.

@kamipo

This comment has been minimized.

Copy link

commented Jun 12, 2019

That is the DHH flavored style, we have not changed any style.

See an example for the style (DHH's PR): rails/rails#35489

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

Hmmm, I wonder why I was under the impression that Rails favours:

class Foo
  def bar
  end

private
  def baz
  end
end

That's actually why I suggested the outdented naming to @koic. I'll have to check this more closely.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

My bad then. Well, I'm out of a good idea for the name then. :D How does @dhh refer to that style?

@koic

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

It seems that my commentary on the PR description was not enough.

I named it EnforcedStyle: indented based on the following Rails coding conventions:

Indent and no blank line after private/protected.

https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#follow-the-coding-conventions

This assumes the following code indicated by @kamipo.

# good
class Foo
  def bar
  end

  private
    def baz
    end
end

AFAIK, the following code is generated at Rails 4.0 using bundle exec rails g scaffold. This is an application code generated by Rails users (In fact, this coding style has been around since ancient times).

% bundle exec rails -v
Rails 4.0.0
% bundle exec rails g scaffold article author:string
% cat app/controllers/articles_controller.rb
class ArticlesController < ApplicationController
  before_action :set_article, only: [:show, :edit, :update, :destroy]

  # GET /articles
  # GET /articles.json
  def index
    @articles = Article.all
  end

  (snip)

  # DELETE /articles/1
  # DELETE /articles/1.json
  def destroy
    @article.destroy
    respond_to do |format|
      format.html { redirect_to articles_url }
      format.json { head :no_content }
    end
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_article
      @article = Article.find(params[:id])
    end

    # Never trust parameters from the scary internet, only allow the white list through.
    def article_params
      params.require(:article).permit(:author)
    end
end
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

@koic I wonder if we shouldn't revert the PR as frankly I can't come up with a better name than Rails. The problem with indented is that it's not clear what we are referring to. Maybe indented_private_methods or something like this? // @rubocop-hq/rubocop-core Help, please! :-)

We can't cut a new release before fixing this.

@koic

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@bbatsov Thanks for the name suggestion. I think indented_internal_methods is a name that represents this style.
@kamipo I think you are a Rails committer familiar with this style. What do you think about this name? I appreciate your expert knowledge in this matter.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2019

@bbatsov Thanks for the name suggestion. I think indented_internal_methods is a name that represents this style.

@koic I'm fine with the proposed name.

koic added a commit to koic/rubocop that referenced this pull request Jun 22, 2019

Rename enforced style for `Layout/IndentationConsistency`
Follow up rubocop-hq#7113 (comment).

This PR renames enforced style for `Layout/IndentationConsistency` from
`EnabledStyle: outdented_access_modifiers` to `EnabledStyle: indented_internal_methods`.

Here is an example style that the name `indented_internal_methods` represents:

```ruby
# good
class Foo
  def bar
  end

  private
    def baz
    end
end
```

@koic koic referenced this pull request Jun 22, 2019

Merged

Rename enforced style for `Layout/IndentationConsistency` #7163

5 of 8 tasks complete
@koic

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

Yeah, I opened a PR #7163 and we may be good to wait for a while to get a better name feedback.

@kamipo

This comment has been minimized.

Copy link

commented Jun 22, 2019

👍 Thank you guys for thinking of about the naming!

bbatsov added a commit that referenced this pull request Jun 22, 2019

Rename enforced style for `Layout/IndentationConsistency`
Follow up #7113 (comment).

This PR renames enforced style for `Layout/IndentationConsistency` from
`EnabledStyle: outdented_access_modifiers` to `EnabledStyle: indented_internal_methods`.

Here is an example style that the name `indented_internal_methods` represents:

```ruby
# good
class Foo
  def bar
  end

  private
    def baz
    end
end
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.