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 have_db_column.with_options misspelled options #1358

Merged
merged 1 commit into from Dec 14, 2020

Conversation

rodriggochaves
Copy link
Contributor

Hi!! This is my first contribuition to shoulda-matchers. If you got any feedback for this PR, I'll be very happy to listen.

This PR looks to solve the bug reported here #1281.

Solution descripton

When there is an argument this matcher doesn't expect, it should raise an ArgumentError. I created a new protected method validate_options to be responsible for this.

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Hi @rodriggochaves — thank you for this! It's nice when PRs are small :) Just had a few comments below.

@@ -137,6 +138,13 @@ def description

protected

OPTIONS = %i(precision limit default null scale primary)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind putting this at the top of the class? That way it "pops" out.

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 moved but I've a question about the visibility of this constant. Shoud I use something to hide like #private_constant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that would make sense if you want to do that.

it 'throws an ArgumentError' do
expect {
have_db_column(:salary).with_options(preccision: 5)
}.to raise_error(ArgumentError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about testing what the error message is going to be rather than what kind of error it is? When you do that you might see that error could be improved a bit. I believe that as invalid_options is an array, when it gets interpolated it will appear like an array in the message. Not sure if that's what you were going after or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Good observation!! I just changed the test and improved the message raised. Let me know what did you think about it?


def validate_options(opts)
invalid_options = opts.keys.map(&:to_sym) - OPTIONS
raise ArgumentError, "Unknown option(s) '#{invalid_options}'" unless invalid_options.empty?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a little style thing: we don't tend to use modifiers or unless — would you mind formatting this as:

if invalid_options.any?
  raise ArgumentError, "Unknown option(s) '#{invalid_options}'"
end

(although see note below about possibly reformatting the message)

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

One more little thing but otherwise I'm good with this!

it 'raises an error with the unknown options' do
expect {
have_db_column(:salary).with_options(preccision: 5, primaryy: true)
}.to raise_error("Unknown option(s): 'preccision, primaryy'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so now that you're testing multiple options, it seems like we could improve this message a bit. What if you change the above to

raise ArgumentError, "Unknown option(s): #{invalid_options.map(&:inspect).join(", ")}"

Then you should get something like:

expect {
  have_db_column(:salary).with_options(preccision: 5, primaryy: true)
}.to raise_error("Unknown option(s): :preccision, :primaryy")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! I improved the message.

have_db_column().with_options() should raise an ArgumentError
if receives a misspelled argument
@KapilSachdev KapilSachdev added this to the 4.5.0 milestone Nov 11, 2020
@mcmire mcmire self-assigned this Dec 9, 2020
Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this. This looks good to me, so thank you! It sounds like we may want to push this to the version after the next immediate one, so I'm going to hold off on merging but this looks ready to go once we are ready to merge.

@VSPPedro
Copy link
Collaborator

Thank you very much, @rodriggochaves!

@mcmire, I noticed that this PR was selected to be part of the 4.5.0. So, I think we can merge it.

@mcmire
Copy link
Collaborator

mcmire commented Dec 14, 2020

@VSPPedro Ah, sorry! For some reason I thought we were inserting a patch release before 4.5.0. In that case I'll merge this.

@mcmire mcmire merged commit 63b6321 into thoughtbot:master Dec 14, 2020
thealiilman added a commit to thealiilman/shoulda-matchers that referenced this pull request Dec 14, 2020
These were introduced after thoughtbot#1358 was merged. Rather bizarrely,
RuboCop didn't run when the PR was opened.
mcmire pushed a commit that referenced this pull request Dec 14, 2020
These were introduced after #1358 was merged. Rather bizarrely,
RuboCop didn't run when the PR was opened.
@VSPPedro VSPPedro mentioned this pull request Dec 28, 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

4 participants