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

Copyedit Rails/RakeEnvironment cop documentation. #164

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

dylnclrk
Copy link
Contributor

@dylnclrk dylnclrk commented Dec 4, 2019

The new Rails/RakeEnvironment task from @pocke is great! 😄 But there are a few copy edits that could make the offense more readable.

This PR tweaks the wording of the offense and supporting documentation to increase readability. This is somewhat subjective, so let me know what you think!


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • [ x 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 Updated existing tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
    • should I do this?
  • 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.

@@ -334,7 +334,7 @@ Rails/Present:
UnlessBlank: true

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

Choose a reason for hiding this comment

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

Maybe also capitalize Rake here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you.

# models, without `environment` dependency.
# This cop checks for Rake tasks without the `environment` dependency. The
# `environment` task loads application code for other Rake tasks. Without
# it, tasks cannot make use application code like models.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing "of" after "use"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@dylnclrk dylnclrk force-pushed the edit-rake-environment-copy branch 2 times, most recently from 1eec660 to 4eb039f Compare December 4, 2019 23:37
Copy link
Contributor

@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!! 👍

# `environment` dependency is important because it loads application code
# for the rake task. The rake task cannot use application code, such as
# models, without `environment` dependency.
# This cop checks for Rake tasks without the `environment` dependency. The
Copy link
Member

Choose a reason for hiding this comment

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

I think that the document will be more consistent with :environment task.

-This cop checks for Rake tasks without the `environment` dependency. The
+This cop checks for Rake tasks without the `:environment` task dependency. The

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. I made the change

# for the rake task. The rake task cannot use application code, such as
# models, without `environment` dependency.
# This cop checks for Rake tasks without the `environment` dependency. The
# `environment` task loads application code for other Rake tasks. Without
Copy link
Member

Choose a reason for hiding this comment

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

-# `environment` task loads application code for other Rake tasks. Without
+# `:environment` task loads application code for other Rake tasks. Without

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

#
# You can ignore the offense if the task satisfies at least one of the
# following conditions:
#
# * The task does not need application code.
# * The task invokes :environment task.
# * The task invokes the :environment task.
Copy link
Member

@koic koic Dec 5, 2019

Choose a reason for hiding this comment

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

-# * The task invokes the :environment task.
+# * The task invokes the `:environment` task.

@dylnclrk dylnclrk force-pushed the edit-rake-environment-copy branch from 4eb039f to 95b1746 Compare December 6, 2019 17:36
* Tweak wording of offense message and supporting documentation to
  increase readability.
@dylnclrk dylnclrk force-pushed the edit-rake-environment-copy branch from 95b1746 to e43c772 Compare December 6, 2019 17:39
@dylnclrk
Copy link
Contributor Author

dylnclrk commented Dec 6, 2019

Should I add a note about this in the changelog?

@koic
Copy link
Member

koic commented Dec 6, 2019

No changelog entry is required for this change. Thank you for the improvement!

@koic koic merged commit 8d48d8d into rubocop:master Dec 6, 2019
@dylnclrk dylnclrk deleted the edit-rake-environment-copy branch December 6, 2019 18:52
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.

4 participants