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

Add Rails/RakeEnvironment cop #130

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Add Rails/RakeEnvironment cop #130

merged 1 commit into from
Nov 4, 2019

Conversation

pocke
Copy link
Contributor

@pocke pocke commented Sep 19, 2019

This pull request will add Rails/RakeEnvironment cop to check rake task definition without :environment dependency.

Please read the cop's documentation comment for the motivation for the cop.

rubocop-rake

I'm developing rubocop-rake. It is a RuboCop plugin for rake. I was trying to implement the cop to rubocop-rake, but I thought it should be implemented to rubocop-rails on second thought.
Because most of the rubocop-rails users can enable the cop, but not all rubocop-rake users need the cop.

auto-correct

I think we can implement auto-correction for the cop, but this pull request omits it. I think it is enough for the first step.

I guess implementing auto-correction is a bit of difficult. Because Rake has many styles to define a task.

task :foo
task foo: :dep
task foo: [:dep1, :dep2]
task :foo, [:arg_name]
taks :foo, [:args_name] => :dep
taks :foo, [:args_name] => [:dep1, :dep2]

Aggressive offense

The cop is not aware of nested dependencies.

task bar: :environment
task foo: :bar

In this case, bar invokse environment, so the cop should not add any offenses to foo.
But the cop is not aware of the nested dependencies. It means the cop does not know bar to invoke environment.

We can choose the following two styles.

  • Do not add any offenses if the task has dependencies.
  • Add an offense even if the task has dependencies.

I choose the latter, which is a more aggressive style, for the first implementation.
I think someone can implement another style in the future if necessary.


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.

@Drenmi
Copy link
Contributor

Drenmi commented Sep 20, 2019

Hm. I tend to have quite a few tasks with other dependencies, e.g.:

task prime: "db:setup"

so the less aggressive style would be nice.

@pocke
Copy link
Contributor Author

pocke commented Sep 20, 2019

so the less aggressive style would be nice.

That is a good point. I agree with you.
I concerned the cop overlooks missing environment without the aggressive style. But the dependency is also checked by the cop in many cases, so totally the cop will not overlook it.

# `foo` task is not checked by the cop
task foo: :bar

# `bar` task is checked by the cop, so totally the cop detects an offense expectedly.
task :bar do
end

So I'll change the style to the less aggressive style.

@pocke
Copy link
Contributor Author

pocke commented Sep 24, 2019

I updated the cop to not add offense aggressively.
If you need, I'll rebase and squash commits.

@koic
Copy link
Member

koic commented Sep 24, 2019

I’d like to review this one. Please wait a moment.

@koic koic self-requested a review September 24, 2019 16:25
@@ -333,6 +333,14 @@ Rails/Present:
# Convert usages of `unless blank?` to `if present?`
UnlessBlank: true

Rails/RakeEnvironment:
Description: 'Set :environment task as a dependency to all rake task.'
Copy link
Member

Choose a reason for hiding this comment

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

Can you enclose :environment in backticks?

# end
#
class RakeEnvironment < Cop
MSG = 'Set :environment task as a dependency to all rake task.'
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Rails/RakeEnvironment:
Description: 'Set :environment task as a dependency to all rake task.'
Enabled: true
VersionAdded: '2.4'
Copy link
Member

Choose a reason for hiding this comment

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

Probably not a problem in most cases, but it is likely that calling :environment task will break a behavior. It may be better to mark it as Safe: false. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. I added Safe: false. Thanks!

@pocke
Copy link
Contributor Author

pocke commented Nov 3, 2019

Thank you for your review! @koic
I updated them.
I'll squash the commits to one if you need.

note: The failing CI builds will be solved by #146

Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Can you squash your commits into one?

@pocke
Copy link
Contributor Author

pocke commented Nov 4, 2019

rebased. THanks!

@koic koic merged commit 9a6058f into rubocop:master Nov 4, 2019
@koic
Copy link
Member

koic commented Nov 4, 2019

Thanks!

@OneDivZero
Copy link

OneDivZero commented Jan 30, 2020

Any suggestions how to fix a no-op task-defintion like this?

# C: Rails/RakeEnvironment: Include :environment task as a dependency for all Rake tasks.
ARGV.each { |a| task a.to_sym do; end }

@andyw8
Copy link
Contributor

andyw8 commented Jan 30, 2020

@OneDivZero As this is a closed PR, I think it would be better to open a new issue for this.

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.

5 participants