Validate confirmation of for integer values #868
Conversation
| expect(builder.record). | ||
| to validate_confirmation_of(builder.attribute_to_confirm) | ||
| [:string, :integer].each do |column_data_type| | ||
| it "passes when data type is #{column_data_type}" do |
| @@ -6,6 +6,7 @@ class RecordValidatingConfirmationBuilder | |||
|
|
|||
| def initialize(options) | |||
| @options = options | |||
| @data_type = options[:data_type].nil? ? :string : options[:data_type] | |||
There was a problem hiding this comment.
Let's do this:
- I believe that I call this option
column_typein other places, so let's rename the option to that. - Let's move this to a method under
private. - Let's use
fetchon Hash to handle the use-value-if-key-given-otherwise-use-another-value logic.
So, all together, that method would look like:
private
def column_type
options.fetch(:column_type, :string)
end* Also added support for all possible column types in ActiveRecord to validate_confirmation_of
| available_column_types.each do |column_type| | ||
| it "passes when the column is of type #{column_type}" do | ||
| builder = builder_for_record_validating_confirmation( | ||
| column_type: column_type |
There was a problem hiding this comment.
Put a comma after the last parameter of a multiline method call.
| when :integer, :float then 2 | ||
| when :decimal then BigDecimal.new(2, 0) | ||
| when :datetime, :time, :timestamp then Time.new(2015, 12, 26, | ||
| 10, 10, 10) |
There was a problem hiding this comment.
Align the parameters of a method call if they span more than one line.
| 26, | ||
| 10, | ||
| 10, | ||
| 10, |
There was a problem hiding this comment.
Align the parameters of a method call if they span more than one line.
| 12, | ||
| 25, | ||
| 10, | ||
| 10, |
There was a problem hiding this comment.
Align the parameters of a method call if they span more than one line.
|
@mcmire I have no idea how to get rid of the HoundCI's violations. Didn't find any example at the style guide. |
|
@shishir127 There was once an example of how to resolve those warnings in our guides, but it and its guideline got removed a while back for some reason. In general, here's the heuristic we tend to use: If you're inside of an argument list, hash, or array, indent by two spaces from the line that opened the argument list, hash, or array. (In other words, don't create a situation where you'd have to tab excessively to indent the line, should you have to do so manually.) So what that means in practice is that instead of saying: this_is_a_long_method(foo,
bar,
baz)you'd say: this_is_a_long_method(
foo,
bar,
baz
)The other heuristic I follow personally is: If you have a situation where you're opening an argument list on a line that's already pretty long, make that method call on the next line. That may allow you to re-style the line in a way that's easier to work with. So coming back to this PR, instead of writing when :datetime, :time, :timestamp then Time.new(
2015,
12,
25,
...
)I might say when :datetime, :time, :timestamp
Time.new(2015, 12, 25, ...) |
|
@mcmire Is this fix not needed anymore? |
|
@shishir127 Sorry for the late reply on this, I've been out. I'm not sure. I'm finding it hard to remember why this fix was necessary in the first place. Can you enlighten me, please? 😬 |
|
It's apparently for #865, to make the validate_confirmation_of matcher work with integer values. TBH I don't remember much about this too. Do you want me to rebase and update the PR? |
|
@shishir127 Yeah, if you could rebase that would definitely help. |
|
Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this PR. Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this PR, then please let us know so that we can reprioritize it. Thanks! |
Fix for #865