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

Add Czech license plates #1565

Merged
merged 7 commits into from
Jul 16, 2021
Merged

Add Czech license plates #1565

merged 7 commits into from
Jul 16, 2021

Conversation

filiptronicek
Copy link
Contributor

@filiptronicek filiptronicek commented Dec 30, 2020

I have added Czech license plates with regards to all the possible cases I found. RegExr with tests

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable) - I think I need help with this one

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #1565 (2322e06) into master (d1a9b6d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1565   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          101       101           
  Lines         1854      1855    +1     
=========================================
+ Hits          1854      1855    +1     
Impacted Files Coverage Δ
src/lib/isLicensePlate.js 100.00% <100.00%> (ø)
src/lib/isPassportNumber.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1a9b6d...2322e06. Read the comment docs.

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Looks good, however, could add a few test cases?

@filiptronicek
Copy link
Contributor Author

@profnandaa yes, totally agreed, but as I mentioned, I am not really comfortable in writing tests here yet, any guide to get me started?

@MoltenCoffee
Copy link

@profnandaa yes, totally agreed, but as I mentioned, I am not really comfortable in writing tests here yet, any guide to get me started?

This should get you started:

test({
      validator: 'isLicensePlate',
      args: ['cs-CZ'],
      valid: [
        'valid-plate1',
        'valid-plate2'
      ],
      invalid: [
        '',
        'invalidlicenseplate',
        'A1-B2-C3',
        'ABC-1-EF',
      ],
    });

Edit and add to /test/validators.js, here

@filiptronicek filiptronicek requested a review from profnandaa May 11, 2021 10:18
@@ -1,6 +1,8 @@
import assertString from './util/assertString';

const validators = {
'cs-CZ': str =>
/^(([ABCDEFHKIJKLMNPRSTUVXYZ]|[0-9])-?){5,8}$/.test(str),
Copy link
Contributor

@fedeci fedeci May 11, 2021

Choose a reason for hiding this comment

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

Suggested change
/^(([ABCDEFHKIJKLMNPRSTUVXYZ]|[0-9])-?){5,8}$/.test(str),
/^[0-9]([A-Z][0-9]{5}|[A-Z]{2}[0-9]{4})$/.test(str),

nit: we can increase accuracy in this way.
Passing examples:

0A18929
0AB1892

This regex does not take into account custom license plates and we have to check it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shortened version looks way better, I agree @fedeci, but this does not include disallowed characters, which include O, G, Q, and W.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to what I understood from wikipedia, those characters are only disallowed in custom license plates🤔

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 think maybe there is conflicting information on the article in English, and the one which is in Czech, this is a paragraph translated from the Czech one:

Today's marks contain five to seven characters, depending on the vehicle type (numbers and letters without accents, except for G, O, Q, W, which could be confused with C, V, and the number 0 or the number 6).

This means, that this applies to all license plates, not just custom ones.

@rubiin
Copy link
Member

rubiin commented Jul 5, 2021

@profnandaa looks stale , lets get it merged if author is still available or I could add in few tests with the new PR

@filiptronicek
Copy link
Contributor Author

filiptronicek commented Jul 5, 2021

Hi, @rubiin, I am still available for any changes or tests to be made. I think there is still an unresolved discussion about whether to handle custom license plates separately or not from the review by @fedeci. If this gets resolved we should be ready to merge. (at least I think so)

@rubiin
Copy link
Member

rubiin commented Jul 5, 2021

Hi, @rubiin, I am still available for any changes or tests to be made. I think there is still an unresolved discussion about whether to handle custom license plates separately or not from the review by @fedeci. If this gets resolved we should be ready to merge. (at least I think so)

Yes please add more tests as said by @profnandaa so we could merge it. This is to limit stale PRs

@filiptronicek
Copy link
Contributor Author

@rubiin added those tests!

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! 🎉

@profnandaa profnandaa merged commit c87956a into validatorjs:master Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants