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 deactivated StyleGuideBaseURL for Layout/ClassStructure #6865

Merged
merged 1 commit into from Mar 28, 2019

Conversation

aeroastro
Copy link
Contributor

StyleGuide config for Layout/ClassStructure was
written as an absolute URL instead of a relative URL.
This prevents custom StyleGuideBaseURL from taking precedence
over original rubocop-hq URL.

https://github.com/rubocop-hq/rubocop/blob/86f35b21a9de380028bc7bc7cf1a6e212106e6b9/config/default.yml#L74-L76

This patch fix the issue by specifying fragment as the
StyleGuide of Layout/ClassStructure.

Since this is a bug fix in default config, I think we can omit a test.
If required, please let me know.


Before submitting the PR make sure the following are checked:

  • Wrote [good commit messages][1].
  • 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.

`StyleGuide` config for `Layout/ClassStructure` was
written as an absolute URL instead of a relative URL.
This prevents custom `StyleGuideBaseURL` from taking precedence
over original rubocop-hq URL.

This patch fix the issue by specifying fragment as the
`StyleGuide` of `Layout/ClassStructure`.
@aeroastro aeroastro force-pushed the feature/remove-preceding-prefix branch from 4e59646 to 344a678 Compare March 27, 2019 14:45
@aeroastro
Copy link
Contributor Author

Sorry, it seems like I had uncommitted changes in my local workspace. Now all the diff is uploaded.

@@ -7,6 +7,7 @@
* [#6855](https://github.com/rubocop-hq/rubocop/pull/6855): Fix an exception in `Rails/RedundantReceiverInWithOptions` when the body is empty. ([@ericsullivan][])
* [#6856](https://github.com/rubocop-hq/rubocop/pull/6856): Fix auto-correction for `Style/BlockComments` when the file is missing a trailing blank line. ([@ericsullivan][])
* [#6858](https://github.com/rubocop-hq/rubocop/issues/6858): Fix an incorrect auto-correct for `Lint/ToJSON` when there are no `to_json` arguments. ([@koic][])
* [#6865](https://github.com/rubocop-hq/rubocop/pull/6865): Fix deactivated `StyleGuideBaseURL` for `Layout/ClassStructure`. ([@aeroastro][])
Copy link
Member

Choose a reason for hiding this comment

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

Change log entry is not required because there is no impact to users.

@aeroastro
Copy link
Contributor Author

Thank you for the quick review. 🐱
Although the impact is small, this actually affect some portion of users. Sorry for not providing detailed information.

Prerequisites

Let's assume we have the following .rubocop.yml and the target test.rb.

AllCops:
  StyleGuideBaseURL: https://github.com/aeroastro/ruby-style-guide
Layout/ClassStructure:
  Enabled: true
# :nodoc:
class Test
  private

  def hoge
  end

  HOGE = 3
end

The reason why I changed the StyleGuideBaseURL is:

  • We are using a project-specific ruby style guide
  • We want all the ruby style guide URL to be linked to the new URL.

c.f #3535

Impact

Then, run rubocop with --display-style-guide option to show the style guide URL.
This generates the following offense messages. (I removed the warn message of Performance Cop for simplicity.)

$ ruby -v
ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-darwin16]
$ bundle exec rubocop -v
0.66.0
$ bundle exec rubocop --display-style-guide test.rb

Before

Inspecting 1 file
C

Offenses:

test.rb:5:3: C: Style/EmptyMethod: Put empty method definitions on a single line. (https://github.com/aeroastro/ruby-style-guide#no-single-line-methods)
  def hoge ...
  ^^^^^^^^
test.rb:8:3: C: Layout/ClassStructure: constants is supposed to appear before private_methods. (https://github.com/rubocop-hq/ruby-style-guide#consistent-classes)
  HOGE = 3
  ^^^^^^^^

1 file inspected, 2 offenses detected

After

Inspecting 1 file
C

Offenses:

test.rb:5:3: C: Style/EmptyMethod: Put empty method definitions on a single line. (https://github.com/aeroastro/ruby-style-guide#no-single-line-methods)
  def hoge ...
  ^^^^^^^^
test.rb:8:3: C: Layout/ClassStructure: constants is supposed to appear before private_methods. (https://github.com/aeroastro/ruby-style-guide#consistent-classes)
  HOGE = 3
  ^^^^^^^^

1 file inspected, 2 offenses detected

You can see that the style guide URL of LayoutClassStructure is now successfully linked to custom ruby style guide, https://github.com/aeroastro/ruby-style-guide#consistent-classes instead of https://github.com/rubocop-hq/ruby-style-guide#consistent-classes

Additional Information

On the current master, only Layout/ClassStructure has duplicate URL definition.
So this fixes the last one.

# on current master
$ grep -C 2 'https://github.com/rubocop-hq/ruby-style-guide' config/default.yml
  # When specifying style guide URLs, any paths and/or fragments will be
  # evaluated relative to the base URL.
  StyleGuideBaseURL: https://github.com/rubocop-hq/ruby-style-guide
  # Extra details are not displayed in offense messages by default. Change
  # behavior by overriding ExtraDetails, or by giving the
--
--
Layout/ClassStructure:
  Description: 'Enforces a configured order of definitions within a class body.'
  StyleGuide: 'https://github.com/rubocop-hq/ruby-style-guide#consistent-classes'
  Enabled: false
  VersionAdded: '0.52'

@koic
Copy link
Member

koic commented Mar 28, 2019

I see. Thanks!

@koic koic merged commit d28f860 into rubocop:master Mar 28, 2019
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