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 option OnlyFor to the Bundler/GemComment cop. #7978

Merged
merged 21 commits into from May 31, 2020

Conversation

ric2b
Copy link
Contributor

@ric2b ric2b commented May 15, 2020

This PR adds a boolean option OnlyFor to the Bundler/GemComment cop.

The goal of this option is to require gem comments only if the gem version has an explicit version specifier or other options like required/github/etc, so that the reason for the specifier/options is documented.

I tested it on a project I work on and it worked as expected.


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.

@ric2b
Copy link
Contributor Author

ric2b commented May 15, 2020

By the way, I'm also planning on doing a similar PR for parameters that change the source of a gem, such as source:, github:, etc (documented here).

I was thinking it could be a bit more general and work by receiving a list of parameter names that should activate the cop, something like: OnlyIfUsingOptions: ['require', 'github']

Does that make sense to you?

@ric2b ric2b changed the title Add option OnlyIfVersionRestricted to the Bumdler/GemComment cop. Add option OnlyIfVersionRestricted to the Bundler/GemComment cop. May 15, 2020
@@ -5,7 +5,7 @@ module Cop
module Bundler
# Add a comment describing each gem in your Gemfile.
#
# @example
# @example OnlyIfVersionRestricted: false (default)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also expand the cop summary.

@bbatsov
Copy link
Collaborator

bbatsov commented May 17, 2020

You should also mention this in the default config. I'm not sure I like the idea of a boolean option, though - might be a better idea to have some styles like "always", "only gems with some extra params". I like your idea overall, but we have to be careful to make the config both easy to understand and extendable.

@ric2b
Copy link
Contributor Author

ric2b commented May 18, 2020

Yeah, making it extensible is a great point.

What do you think of a configuration with three parameters:

  • AllGems: Boolean
  • GemsWithVersionSpecifiers: Boolean
  • GemsWithAnyOfTheseOptions: Array

Where AllGems: true would ignore the other options and apply to all gems. Otherwise the rest of the options would be combined.

As an example:

Bundler/GemComment:
  Enabled: true
  AllGems: false
  GemsWithVersionSpecifiers: true
  GemsWithAnyOfTheseOptions: 
    - github
    - bitbucket

Would only trigger for a gem that has version restrictions OR uses at least one of github: and bitbucket:.

@bbatsov
Copy link
Collaborator

bbatsov commented May 19, 2020

The problem with the boolean flags approach is that some of them are not compatible with one another and this makes the configuration harder. A different approach might be to just have a list of things to check - e.g.:

- Check
  - gems_with_version
  - gems_from_source
  - etc

And I guess when empty this can mean "all". Or there can be an enforced style "All" and "Subset" and the second one would consult this extra configuration option. Not sure what's the best approach, but I really don't like the boolean flags, so I'd rather us avoid it.

@ric2b
Copy link
Contributor Author

ric2b commented May 19, 2020

My last suggestion would make the options compatible with each other, it's just that gems_with_version would be a separate option because it's not a kwarg.

But I like your suggestion, it makes the configuration more straightforward, just need to treat version specifiers as a special case 👍

@bbatsov
Copy link
Collaborator

bbatsov commented May 21, 2020

Fine by me.

@ric2b
Copy link
Contributor Author

ric2b commented May 21, 2020

@bbatsov The new version has a single new configuration: OnlyWhenUsingAnyOf.

If it's empty the cop behaves the same as before, checking all gems, otherwise it only registers offenses on gems with one or more of the configured options.

with_version_specifiers can also be passed as an option in to check for version specifiers.

Happy to change the names to whatever you prefer, if you don't think they're clear enough or consistent with rubocop conventions.

CHANGELOG.md Outdated Show resolved Hide resolved
lib/rubocop/cop/bundler/gem_comment.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/bundler/gem_comment.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/bundler/gem_comment_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/bundler/gem_comment_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/bundler/gem_comment_spec.rb Show resolved Hide resolved
ric2b and others added 6 commits May 24, 2020 15:03
Co-authored-by: mwolman <33032571+mwolman@users.noreply.github.com>
Co-authored-by: mwolman <33032571+mwolman@users.noreply.github.com>
Co-authored-by: mwolman <33032571+mwolman@users.noreply.github.com>
Co-authored-by: mwolman <33032571+mwolman@users.noreply.github.com>
@ric2b ric2b changed the title Add option OnlyIfVersionRestricted to the Bundler/GemComment cop. Add option OnlyWhenUsingAnyOf to the Bundler/GemComment cop. May 27, 2020
@ric2b ric2b requested a review from bbatsov May 30, 2020 19:51
@ric2b
Copy link
Contributor Author

ric2b commented May 30, 2020

All the tests pass on my machine, the failing test seems completely unrelated to this PR, not sure what to do.

config/default.yml Outdated Show resolved Hide resolved
@bbatsov
Copy link
Collaborator

bbatsov commented May 31, 2020

I think it's a good idea to summarize in docs somewhere all the common params (https://bundler.io/man/gemfile.5.html) for its easier for people to figure out what's available to them.

@bbatsov
Copy link
Collaborator

bbatsov commented May 31, 2020

You should also update VersionChanged to 0.85.

@bbatsov bbatsov merged commit 8c3dd0b into rubocop:master May 31, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented May 31, 2020

Thanks!

@ric2b ric2b changed the title Add option OnlyWhenUsingAnyOf to the Bundler/GemComment cop. Add option OnlyFor to the Bundler/GemComment cop. May 31, 2020
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

3 participants