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

New cop Rails/DelegateAllowBlank #3434

Merged
merged 1 commit into from Oct 12, 2016

Conversation

Projects
None yet
3 participants
@connorjacobsen
Contributor

connorjacobsen commented Aug 22, 2016

Adds a new cop to catch usage of allow_blank as an option to ActiveSupport's delegate method. allow_blank is not a supported option and fails silently. The correct option is allow_nil. Other parts of Rails have allow_blank as an option, such as some validations.

Please let me know if there are more idiomatic ways to do this, especially using def_node_matcher.


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).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Aug 23, 2016

Contributor

Mmm, I think this is out of scope, wouldn't it be better if ActiveSupport detected invalid usage?

Contributor

deivid-rodriguez commented Aug 23, 2016

Mmm, I think this is out of scope, wouldn't it be better if ActiveSupport detected invalid usage?

@connorjacobsen

This comment has been minimized.

Show comment
Hide comment
@connorjacobsen

connorjacobsen Aug 23, 2016

Contributor

The same argument could be made for all of the Rails cops, no?

Contributor

connorjacobsen commented Aug 23, 2016

The same argument could be made for all of the Rails cops, no?

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Aug 23, 2016

Contributor

Most of Rails cops choose a preferred style over a few valid, non-deprecated styles. In this case, it would be detecting invalid usage. But I guess if this is a very common mistake, then it's fine. Maybe I just don't like it because I've never run into this confusion.

Contributor

deivid-rodriguez commented Aug 23, 2016

Most of Rails cops choose a preferred style over a few valid, non-deprecated styles. In this case, it would be detecting invalid usage. But I guess if this is a very common mistake, then it's fine. Maybe I just don't like it because I've never run into this confusion.

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Aug 23, 2016

Contributor

Rails/NotNullColumn is definitely similar. Not my cup of tea, but if the other one was accepted by the mantainers, maybe this will be accepted as well :)

Contributor

deivid-rodriguez commented Aug 23, 2016

Rails/NotNullColumn is definitely similar. Not my cup of tea, but if the other one was accepted by the mantainers, maybe this will be accepted as well :)

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 15, 2016

Collaborator

I think that https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/rails/not_null_column.rb and https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/rails/save_bang.rb are kind of similar

I agree with this. The question is, however, how common is the mistake this cop is aiming to prevent. We can't really have cops for every method that accepts arguments via a hash.

Collaborator

bbatsov commented Sep 15, 2016

I think that https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/rails/not_null_column.rb and https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/rails/save_bang.rb are kind of similar

I agree with this. The question is, however, how common is the mistake this cop is aiming to prevent. We can't really have cops for every method that accepts arguments via a hash.

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Sep 15, 2016

Contributor

The idea behind the allow_nil delegate option is to delegate via try so delegations on nil objects don't raise but just return nil.

The idea behind the allow_blank option in rails validators is to consider blank objects as valid, regardless of the validator's result.

Apparently some people mistakenly use allow_blank for Rails delegate, but I don't know how common that is.

Contributor

deivid-rodriguez commented Sep 15, 2016

The idea behind the allow_nil delegate option is to delegate via try so delegations on nil objects don't raise but just return nil.

The idea behind the allow_blank option in rails validators is to consider blank objects as valid, regardless of the validator's result.

Apparently some people mistakenly use allow_blank for Rails delegate, but I don't know how common that is.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 5, 2016

Collaborator

@connorjacobsen ping :-)

Collaborator

bbatsov commented Oct 5, 2016

@connorjacobsen ping :-)

@connorjacobsen

This comment has been minimized.

Show comment
Hide comment
@connorjacobsen

connorjacobsen Oct 5, 2016

Contributor

@bbatsov What are your thoughts?

Contributor

connorjacobsen commented Oct 5, 2016

@bbatsov What are your thoughts?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 5, 2016

Collaborator

I was wondering how common do you think this problem is in the wild?

Collaborator

bbatsov commented Oct 5, 2016

I was wondering how common do you think this problem is in the wild?

@connorjacobsen

This comment has been minimized.

Show comment
Hide comment
@connorjacobsen

connorjacobsen Oct 5, 2016

Contributor

Honestly, I'm not sure. Any guesstimate I made would just being pulling numbers out of thin air.

Contributor

connorjacobsen commented Oct 5, 2016

Honestly, I'm not sure. Any guesstimate I made would just being pulling numbers out of thin air.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 6, 2016

Collaborator

Well, OK. Rebase this PR and update the changelog for the current version and let's have this merged. Hopefully it will be useful.

Collaborator

bbatsov commented Oct 6, 2016

Well, OK. Rebase this PR and update the changelog for the current version and let's have this merged. Hopefully it will be useful.

@connorjacobsen

This comment has been minimized.

Show comment
Hide comment
@connorjacobsen

connorjacobsen Oct 12, 2016

Contributor

@bbatsov Changes made, ready for another pass.

Contributor

connorjacobsen commented Oct 12, 2016

@bbatsov Changes made, ready for another pass.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 12, 2016

Collaborator

@connorjacobsen The build is failing.

Collaborator

bbatsov commented Oct 12, 2016

@connorjacobsen The build is failing.

@connorjacobsen

This comment has been minimized.

Show comment
Hide comment
@connorjacobsen

connorjacobsen Oct 12, 2016

Contributor

Sorry about that. Fixed.

Contributor

connorjacobsen commented Oct 12, 2016

Sorry about that. Fixed.

@bbatsov bbatsov merged commit 795841d into rubocop-hq:master Oct 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment