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

feat(isLicensePlate): updated German license plate validation #1945

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

bennetfabian
Copy link
Contributor

In this PR I updated all the available area codes (https://de.wikipedia.org/wiki/Liste_der_Kfz-Kennzeichen_in_Deutschland) for Germany's license plate validation regex.

Also I decided it will be easier to update and maintain in the future if all area codes are in alphabetical order. There are no performance or speed drawbacks with alphabetical sorting.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@bennetfabian bennetfabian changed the title Updated German license plate regex feat(isLicensePlate): updated German license plate validation Mar 25, 2022
Forgot to remove the old regex in my last commit
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #1945 (290debb) into master (c1b21a9) will not change coverage.
The diff coverage is n/a.

❗ Current head 290debb differs from pull request most recent head 080f7df. Consider uploading reports for the commit 080f7df to get more accurate results

@@            Coverage Diff            @@
##            master     #1945   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       102           
  Lines         2085      2085           
  Branches       472       472           
=========================================
  Hits          2085      2085           
Impacted Files Coverage Δ
src/lib/isLicensePlate.js 100.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 c1b21a9...080f7df. Read the comment docs.

@bennetfabian
Copy link
Contributor Author

I'd like kindly ask you to take a look at my change. Only carefully switched out the area code part of the regex so it should be safe to say it isn't breaking anything.
@chriso @profnandaa @ezkemboi @tux-tn

Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

please add tests for the new code as well

@bennetfabian
Copy link
Contributor Author

please add tests for the new code as well

Sorry for disagreeing but exactly why?
I understand the importance of tests but there are already 22 test values supposed to be valid and 5 supposed to be invalid.
Yes, of course, I can think of a few more but especially when comparing to other methods that have less tests I think 22 valid & 5 invalid is more than solid enough.

So why should there be more?
The format remains the same as my PR doesn't even touch the format and structure validation but only updates the available area/district codes.

———

But thanks for commenting. I had another look at the regex and there is one obsolete | operator. Will fix in the next hour.

@rubiin
Copy link
Member

rubiin commented Mar 25, 2022

Well the thing is since there is a code update, the new tests would ensure that the change agrees to the final output. Otherwise things would break in future. Also it would help review the code more fatser

@bennetfabian
Copy link
Contributor Author

Well the thing is since there is a code update, the new tests would ensure that the change agrees to the final output. Otherwise things would break in future. Also it would help review the code more fatser

Okay, I agree. Better safe than sorry. Will think of a few more tests.

@rubiin
Copy link
Member

rubiin commented Mar 25, 2022

Well the thing is since there is a code update, the new tests would ensure that the change agrees to the final output. Otherwise things would break in future. Also it would help review the code more fatser

Okay, I agree. Better safe than sorry. Will think of a few more tests.

Yeah thats the reason :)

@bennetfabian
Copy link
Contributor Author

@ezkemboi Could you merge please?

@rubiin rubiin merged commit eaca48e into validatorjs:master Jul 18, 2022
@rubiin
Copy link
Member

rubiin commented Jul 18, 2022

Thankyou for the contribution 🎉

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.

2 participants