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

Added support for Vietnamese Citizen Identity Card Number #570

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

git03-Nguyen
Copy link

Hello, this PR added extension method to generate Vietnamese 12-digit Citizen Identity Card Number ("Căn cước công dân - CCCD" in local language):

  • Add extension method Cccd() on Person object in Bogus.Extensions.Vietnam namespace.
  • Add unit tests.
  • Modify README.md.

@bchavez
Copy link
Owner

bchavez commented Nov 5, 2024

Great work. Thank you for this pull request. I'll try to get to this soon, but might be a little difficult with the holidays coming up, I'll be at the airport in a few days and hopefully I will try to review this while I wait at the terminal

@git03-Nguyen
Copy link
Author

git03-Nguyen commented Nov 5, 2024

Great work. Thank you for this pull request. I'll try to get to this soon, but might be a little difficult with the holidays coming up, I'll be at the airport in a few days and hopefully I will try to review this while I wait at the terminal

Happy to hear that ^^. It's just some simple work done in my spare time cuz I really love Bogus.
Anw, tell me if the code needs any changes.

@bchavez
Copy link
Owner

bchavez commented Nov 8, 2024

Hi @git03-Nguyen; thanks again for the PR.

After reviewing this PR at the airport, it appears we are slightly misaligned with some of city names which could potentially throw some exceptions when this code executes.

I diffed the following vi locale city names from our JSON:

then compared it to the CityCodes in this PR, and we have a few city names that look like they might not match;

image

image

We should probably get these two lists corrected and synchronized so we don't stumble on an exception with throw new ArgumentException($"City {p.Address.City} is not supported." when Person.Cccd() executes.

Let me know what you think.

@git03-Nguyen
Copy link
Author

Hi @bchavez , thank you so much for your review, and I apologize for my negligence.
Actually, I did compare the lists initially and fixed some differences, but I only gave them a quick glance, it was my mistake not using proper diff tools.

I’ve now pushed two commits:

  • The first fixes the city names "Khánh Hoà", "Thừa Thiên-Huế", and "Thanh Hoá", and includes related unit test cases.
  • The second generates the city names using the Bogus data "vi.locale.json" and adds exhaustive tests for all 63 cities/provinces in Vietnam. If you feel it’s unnecessary, we can revert that commit.

Please let me know if there’s anything else you'd like me to adjust. I really appreciate your feedback! <3

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