check vin type, length, and forbidden characters #1
Conversation
|
Greetings Daniel. I've had a look on your PR and I like the idea of checking for valid vins for every call to parsing functions, but I don't like having the valid function defined in two places. Is |
|
I found your implementation later after I commited, we can merge these two
together.
I was also thinking to have a optional param that enables the checks. This
would be false by default. This way it may be specified what kind of check,
if any, the user may want to enable (we have vins with spaces in our app.
Currently it passes the tests with these in your lib)
…On Thu, 30 Jul 2020, 22:21 nighcoder, ***@***.***> wrote:
Greetings Daniel.
Thanks for your feedback.
I've had a look on your PR and I like the idea of checking for valid vins
for every call to parsing functions, but I don't like having the valid
function defined in two places. Is is_valid fn failing on your tests?
One thing is_valid does not check and your implementation does is to
check if argument is string. So maybe we can add the string check to
is_valid and use that in the valid wrapper?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG4X4YNCBK76IT4VN7IIATR6HI33ANCNFSM4POOGQBA>
.
|
|
The decorator can take the optional param. With a set of options O(1) access time and no order. Then in every if test you just add as first argument 'set contains param and' current test then check I , if param won't be in set then it will be skipped. |
|
Or it can also use optional kwargs - its more pythonic this way when I think about it. Still can accept a dict of **options, then it should work with editor code completion |
|
|
I like the idea of adding extra arguments to parsing fns to fine tune their behaviour. I would remove the I was aware that most fns (except I'm not sure I follow you about the checks. The only optional check I see when determining the validity is the check digit which is only enforced for NA vehicles. All the other checks (len = 17, is string, only valid chars) I see as non optional. Other thing to note, if we're going to use |
|
You're right, maybe most of the checks don't make sense as optional but the
17 chars I think should be opt out:
- I work currently with canadian black book data, they do valuation and
sometimes are returning just part of vin number. I think is scoped to the
model not the exact car. forcing 17 chars would force to add this chars
every time. sending a partial vin to them is also allowed.
- Imagine an auto complete field on a webpage- it can parse partial input
and display to the user in real time without additional processing. So
current behavior may be fine in some cases.
A an explanation why I started this-we have mocked data that looks like:
vin0000000000123-it was failing when I used one of the functions to check
if its valid due to "I" is not a valid character and the checksum
calculation failed I think.
…On Fri, 31 Jul 2020, 01:50 nighcoder, ***@***.***> wrote:
I like the idea of adding extra arguments to parsing fns to fine tune
their behaviour. I would remove the upper wrapper and add an
ignore_case=True extra param to each function.
I was aware that most fns (except parser) don't check the validity of the
input before parsing it, but didn't think of it as an issue at the time. I
do realize now that returning a result for a junk string is error prone and
confusing. So I'm wondering now if it's any value to adding the possibility
of opting-out of input validation check.
I'm not sure I follow you about the checks. The only optional check I see
when determining the validity is the check digit which is only enforced for
NA vehicles. All the other checks (len = 17, is string, only valid chars) I
see as non optional.
Other thing to note, if we're going to use is_valid as from inside the
parser fns, we need to make it stop depending on country and its corner
case where it returns unassigned.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG4X43NWXVSXXPQY5S4GTTR6IBNPANCNFSM4POOGQBA>
.
|
|
You make a good point about parsing partial input data. I've checked some functions with your example and some functions return something, others return None. That doesn't feel right I've checked a bit the function in question to check their requirements and I've found a minimal requirements for their inputs:
Some functions have additional requirements: like a minimal number of characters, or like check_valid that only work for North America. |
nighcoder
left a comment
nighcoder
left a comment
There was a problem hiding this comment.
I think valid should return None if we're getting bad data. That way we can differentiate between False returned by a predicate and False returned by the wrapper.
As for the wrapper itself:
- string check is ok
- length check change to >17 to allow for partial vins
- IOQ check is not enough... we have the whole unicode set to filter out. Use the CHARS constant to filter the input or just copy the relevant part from is_valid
- I would also add the check that string does not start with 0.
2 participants
cool lib but is failing for me on some mocked vins with invalid characters - can we add this checks ?