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 for new_cop rake task #6645

Merged
merged 5 commits into from
Jan 22, 2019
Merged

Fix for new_cop rake task #6645

merged 5 commits into from
Jan 22, 2019

Conversation

francoisa
Copy link
Contributor

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.


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.

@francoisa
Copy link
Contributor Author

I've never submitted a PR to a github repo before and I am not an experienced ruby developer, but the new_cop rake task would fail with:

rake aborted!
TypeError: no implicit conversion from nil to integer
.../rubocop-hq/rubocop/lib/rubocop/cop/generator/configuration_injector.rb:26:in `insert'
.../rubocop-hq/rubocop/lib/rubocop/cop/generator/configuration_injector.rb:26:in `inject'
.../rubocop-hq/rubocop/lib/rubocop/cop/generator.rb:136:in `inject_config'
tasks/new_cop.rake:20:in `block in <top (required)>'
.../2.5.1/bin/bundle:23:in `load'
.../2.5.1/bin/bundle:23:in `<main>'
Tasks: TOP => new_cop
(See full trace by running task with --trace)

until I put this fix in.

@wata727
Copy link
Contributor

wata727 commented Jan 13, 2019

@francoisa Thanks for your contribution. I tried the new_cop rake task, but I cannot reproduce the above error...

% bundle exec rake new_cop\[Rails/Test\]
[create] lib/rubocop/cop/rails/test.rb
[create] spec/rubocop/cop/rails/test_spec.rb
[modify] lib/rubocop.rb - `require_relative 'rubocop/cop/rails/test'` was injected.
[modify] A configuration for the cop is added into config/default.yml.
         If you want to disable the cop by default, set `Enabled` option to false.
Do 3 steps:
  1. Add an entry to the "New features" section in CHANGELOG.md,
     e.g. "Add new `Rails/Test` cop. ([@your_id][])"
  2. Modify the description of Rails/Test in config/default.yml
  3. Implement your new cop in the generated file!

FWIW, if config/default.yml is empty, it seems that a similar error occurs.

% :> config/default.yml
% bundle exec rake new_cop\[Rails/Test\]
[create] lib/rubocop/cop/rails/test.rb
[create] spec/rubocop/cop/rails/test_spec.rb
[modify] lib/rubocop.rb - `require_relative 'rubocop/cop/rails/test'` was injected.
rake aborted!
TypeError: no implicit conversion from nil to integer
/Users/watanabekazuma/workspace/ruby/rubocop/lib/rubocop/cop/generator/configuration_injector.rb:26:in `insert'
/Users/watanabekazuma/workspace/ruby/rubocop/lib/rubocop/cop/generator/configuration_injector.rb:26:in `inject'
/Users/watanabekazuma/workspace/ruby/rubocop/lib/rubocop/cop/generator.rb:136:in `inject_config'
tasks/new_cop.rake:20:in `block in <top (required)>'
/Users/watanabekazuma/.rbenv/versions/2.4.1/bin/bundle:22:in `load'
/Users/watanabekazuma/.rbenv/versions/2.4.1/bin/bundle:22:in `<main>'
Tasks: TOP => new_cop
(See full trace by running task with --trace)

Please check again whether the problem is reproduced. Also, make sure that config/default.yml has not been changed.

@pocke
Copy link
Collaborator

pocke commented Jan 13, 2019

@francoisa Can you show me the full command when it's errored? e.g. bundle exec rake 'new_cop[Foo/Bar]'

I got the same error when the added cop's department does not exist, and the department is the last of dictionary order.
For example:

$ bundle exec rake new_cop[Zzz/Aaa]
/home/pocke/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/lexer.rb:10836: warning: assigned but unused variable - testEof
rake aborted!
TypeError: no implicit conversion from nil to integer
/home/pocke/ghq/github.com/rubocop-hq/rubocop/lib/rubocop/cop/generator/configuration_injector.rb:26:in `insert'
/home/pocke/ghq/github.com/rubocop-hq/rubocop/lib/rubocop/cop/generator/configuration_injector.rb:26:in `inject'
/home/pocke/ghq/github.com/rubocop-hq/rubocop/lib/rubocop/cop/generator.rb:136:in `inject_config'
tasks/new_cop.rake:20:in `block in <top (required)>'
/home/pocke/.rbenv/versions/trunk/bin/bundle:23:in `load'
/home/pocke/.rbenv/versions/trunk/bin/bundle:23:in `<main>'
Tasks: TOP => new_cop
(See full trace by running task with --trace)
[create] lib/rubocop/cop/zzz/aaa.rb
[create] spec/rubocop/cop/zzz/aaa_spec.rb

@francoisa
Copy link
Contributor Author

Sorry for taking so long to reply. The command I used was:

bundle exec rake new_cop[blaw/settings]

I am using ruby 2.5.3. The steps I took were:

  1. Forked rubocop
  2. Cloned rubocop (on a macos mojave machine)
  3. Changed to the rubocop directory
    4, Ran the command above

@pocke
Copy link
Collaborator

pocke commented Jan 16, 2019

Thank you.
It is the same problem with my explain. Because there are lowercase letters after uppercase letters in ASCII code.

I think this pull request has only a few problem.
It injects the new cop's configuration to bottom of the file, but it actually inject the config before the last cop's config with this patch.

For example:

$ bundle exec  rake 'new_cop[Zzz/Aaa]'
$ git diff
diff --git a/config/default.yml b/config/default.yml
index da0926a63..e0be7a1a3 100644
--- a/config/default.yml
+++ b/config/default.yml
@@ -4304,6 +4304,11 @@ Style/YodaCondition:
   VersionAdded: '0.49'
   VersionChanged: '0.50'
 
+Zzz/Aaa:
+  Description: 'TODO: Write a description of the cop.'
+  Enabled: true
+  VersionAdded: '0.63'
+
 Style/ZeroLengthPredicate:
   Description: 'Use #empty? when testing for objects of length 0.'
   Enabled: true

I guess find_target_line can return configuration_entries.size -1 instead of last_line variable to avoid this problem.

@francoisa
Copy link
Contributor Author

Code refactored.

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 22, 2019

@pocke Are you okay with the changes? I'm ready to merge this if you think it's okay. 🙂

Copy link
Collaborator

@pocke pocke left a comment

Choose a reason for hiding this comment

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

Thank you!

@pocke
Copy link
Collaborator

pocke commented Jan 22, 2019

The build on AppVeyor is faled, but I think we can ignore the failure because it only a network problem.

fatal: unable to access 'https://github.com/rubocop-hq/rubocop.git/': Could not resolve host: github.com
https://ci.appveyor.com/project/bbatsov/rubocop/builds/21785086/job/k076e1p4ssm93o7s

@Drenmi I think it's ready to merge. Thanks!

@Drenmi Drenmi merged commit 08ca192 into rubocop:master Jan 22, 2019
@Drenmi
Copy link
Collaborator

Drenmi commented Jan 22, 2019

Thank you, @francoisa! 🙇

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

4 participants