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

Clean PR 1014 relating to fixing BR postal Code #1082

Merged
merged 3 commits into from Aug 12, 2019

Conversation

ezkemboi
Copy link
Member

Cleans issues arose from PR #1014.

@ezkemboi ezkemboi force-pushed the chore-clean-1014 branch 4 times, most recently from aa26332 to 8f87034 Compare August 11, 2019 09:09
@@ -12,7 +12,7 @@ const patterns = {
AU: fourDigit,
BE: fourDigit,
BG: fourDigit,
BR: /^\d{5}-\d{3}$/,
BR: /^\d{5}(-\d{3})$/,
Copy link
Member Author

@ezkemboi ezkemboi Aug 11, 2019

Choose a reason for hiding this comment

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

@bloodf added a regex where it made sure that it will allow -, . , | as a separator.
From my own research, I realized there are no cases where the separator was not -.
That is why I removed some of his changes.

Cc. @profnandaa.

Copy link
Member

Choose a reason for hiding this comment

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

What's the use of the brackets (, )?

Copy link
Member Author

Choose a reason for hiding this comment

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

To catch the

-\d{3}

as a group.

Copy link
Member Author

Choose a reason for hiding this comment

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

But, I don't know what it means by , you have made in the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Was wondering why we need to catch the group, I'm I missing something? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true @profnandaa, I have updated it.

@@ -6719,6 +6719,14 @@ describe('Validators', () => {
'22040-020',
'39400-152',
],
invalid: [
Copy link
Member Author

Choose a reason for hiding this comment

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

I also made sure that I added tests for invalid cases. Just for safety.

Cc. @profnandaa.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

- fix issues relating to BR postal code
- add more tests
- closes validatorjs#1014
@ezkemboi
Copy link
Member Author

@profnandaa, it seems that I have reverted the changes made by @bloodf.
It seems that the existing logic was valid.

@ezkemboi
Copy link
Member Author

@profnandaa Refer to c5c0bf2
and PR #1049 Done by @tevaum.
I think he cleaned the PR done by @bloodf but did not referenced it here.
My thoughts are true to some point as
@bloodf raised the PR on April and @tevaum raised it in June.

@profnandaa
Copy link
Member

Good catch @ezrqnkemboi

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.

Adds invalid test cases 👍

@ezkemboi
Copy link
Member Author

Yea, sure @profnandaa.
The good thing is that @bloodf credits will be reflected.
I hope so.

@profnandaa profnandaa merged commit c265581 into validatorjs:master Aug 12, 2019
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.

None yet

3 participants