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(dui validation): allows DUI value to optionally contain hyphen #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joseaquino
Copy link

This change allows for the hyphen in the DUI value to be optional, this two examples are valid:

import { isDUI } from 'sivar-utils'

const validDUI = isDUI('02495046-3')
const alsoValidDUI = isDUI('024950463')

Additionally I simplified the iteration logic of the digits, to make it a little more transparent on what is happening.

@jonathanpalma jonathanpalma self-assigned this Oct 5, 2020
@jonathanpalma jonathanpalma added the enhancement New feature or request label Oct 5, 2020
Copy link
Owner

@jonathanpalma jonathanpalma left a comment

Choose a reason for hiding this comment

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

Hi @joseaquino! Thank you for your contribution, this idea is great, however, I would like to request a few changes:

I do like your approach but since this project is only targetting JS/TS I would like to keep a transparent reference for people using other programming languages in case they need to replicate these algorithms, being said that, I would rather avoid using array helpers.

In order to create an easy and consistent solution, I would suggest you create a function to mask the string based on a regex and then apply the same algorithm.

Apart from that, I would appreciate it if you can include some notes and examples in the readme!

Thanks again, I appreciate you are taking the time to contribute to this project, feel free to reach me out in case you have any questions

@jonathanpalma
Copy link
Owner

Hi @joseaquino, it seems @alepaz created a similar feature for NITs, can you please take a look at his approach at #15?

@joseaquino
Copy link
Author

Hey @jonathanpalma I just have a couple question in regards to your first comments:

  1. When you say "I would rather avoid using array helpers." you are referring to not using array methods like reduce, filter or map?
  2. I am not clear on what you mean by this "I would suggest you create a function to mask the string based on a regex and then apply the same algorithm", as no masking is done in the logic, just capturing patterns with the regex expression and then destructuring them if matches are found. Can you provide a small example of what you mean, please.

With regard to your second comment, I took a look at the PR #15 you mentioned and I personally believe that the logic I propose is simpler and more flexible, as the inclusion/exclusion of the - (hyphens) is handled by the regex expression itself instead of logically trying to check if the string contains them by doing this:

const [municipality, birthdate, correlative, verifier] = str.includes('-') ? str.split('-') : splitNIT(str);

Also the regex expression proposed in #15 only allows either no hyphens at all or all of the hyphens to be present, this logic is not that flexible as it will not work with a NIT value that has partial hyphens like this 0614080803-1114.

@jonathanpalma
Copy link
Owner

Hi @joseaquino, my apologies for the late reply,

1. When you say "I would rather avoid using array helpers." you are referring to not using array methods like `reduce`, `filter` or `map`?

Yes, as I mentioned in my previous comment, I would like this library to work as a reference for people looking for implementing the same solution that are using other languages

2. I am not clear on what you mean by this "I would suggest you create a function to mask the string based on a regex and then apply the same algorithm", as no masking is done in the logic, just capturing patterns with the regex expression and then destructuring them if matches are found. Can you provide a small example of what you mean, please.

I was envisioning a solution that could take 06140808031114 mask it to 0614-080803-111-4 and apply the exact same algorithm (however, it was just a rough proposal to give you an idea of what's this lib main goal 😅)

Don't get me wrong, I do think your solution is great, however, as I quoted in the previous answer I want this library to be easy to replicate in other languages in order to help developers from different communities.

With regard to your second comment, I took a look at the PR #15 you mentioned and I personally believe that the logic I propose is simpler and more flexible, as the inclusion/exclusion of the - (hyphens) is handled by the regex expression itself instead of logically trying to check if the string contains them by doing this:

const [municipality, birthdate, correlative, verifier] = str.includes('-') ? str.split('-') : splitNIT(str);

I guess this statement is subjective to who you ask, if it is a JS developer you might be right, but, again, take into account there might be developers with a different background trying to replicate these algorithms in other languages (in case there isn't any library providing this set of utilities in that specific one)

Also the regex expression proposed in #15 only allows either no hyphens at all or all of the hyphens to be present, this logic is not that flexible as it will not work with a NIT value that has partial hyphens like this 0614080803-1114.

This is something I would personally like to restrict. It should only allows no hyphens at all 06140808031114 or all of the hyphens in place 0614-080803-111-4

Thanks again for contributing, I appreciate your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants