Skip to content

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 bennetfabian requested a review from rubiin March 25, 2022 16:31
@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