Fix Vehicle VIN containing dash character #2559
Conversation
The regression was introduced in #d113aabf33 when additional RuboCop cops were enabled which resulted in `0-0` being corrected to `0-` which is not the same as the former denotes a range whereas the latter is simply the chars `0` and `-`. Looking at the Wikipedia page for plant identifiers it’s not clear what characters position 11 should be limited to but using `0-9` is a rather safe assumption. Though, using `[A-Z0-9]` would probably work as well.
|
@koic Maybe you’re the right person to review this? |
|
@stefannibrasil As you added the PR: Needs Tests tag, do you have a suggestion what exactly needs testing here? The existing tests use the VIN Regex to generate the fake data to begin with (s. |
|
See #2562 |
|
hi @ur5us thank you for initiating this discussion. I was short in time and didn't leave more context besides adding the label. I am mainly concerned about those breaking changes not being caught up by the tests at all. I don't believe we need to test the regex in detail. But it would be nice to have some sort of checks that would help us identify potential issues that cause these types of bugs. I apologize as I don't have an answer right now. I am hoping to have more time this weekend to do that but it looks like @rmm5t is addressing this in a different PR. |
The regression was introduced in #d113aabf33 when additional RuboCop
cops were enabled which resulted in
0-0being corrected to0-whichis not the same as the former denotes a range whereas the latter is
simply the chars
0and-.Looking at the Wikipedia page for plant identifiers it’s not clear what
characters position 11 should be limited to but using
0-9is a rathersafe assumption. Though, using
[A-Z0-9]would probably work as well.