Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add ChileRut.full_formatted_rut #2460
Add ChileRut.full_formatted_rut #2460
Changes from 4 commits
3c22cc2
dd851a2
2bfb3e0
2e18995
a10d535
9db24fb
c62c5b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of following the same pattern we use when generating ID Numbers? i.e.:
That way we could simply have a
formatted:
keyword in the keywordfull_rut
method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the pattern and the rule I will follow it. Please confirm and I can merge this method with the existing one
full_rut
, using theformatted:
argument to switch from full rut or full_formatted_rut. I can follow the brazilian citizen number as an example. I believe I'll have to rewrite the tests, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with the pattern to keep things tidy. About the tests: yes, you'll have to update it. I would follow the Brazilian ID scenarios as seen here.
In this case, we would need cases for testing:
fixed
true and false casesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is a new geneator, there souldn't be any legacy params to pass in. These can just be keyword params, and the
warn_for_deprecated_arguments
can be removed as wellThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to ask that on my review, thanks for the comment @Zeragamba!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I don't quite understand, is this related with your first commit here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing because the other methods handle this legacy/deprecation stuff :/ It's going to be removed soon.
What @Zeragamba means is that for new methods, you don't need the legacy params and the arn_for_deprecated_arguments` block. See this previous PR merging a similar feature without the legacy/deprecation checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what
has_dv
means here. Could you please clarify what is your intention here? I couldn't figure out why the test hasdv
in the name when the implementation doesn't.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DV stands for "digito verificador", "verifying digit" will be the translation, I guess. DV is some kind of checksum of chilean citizen number (rut). There is a mathematical formula described here, you take the citizen number before hyphen as input, and the result is a number between 1-11. If the output is 10, DV = K, if the output is 11, DV = 0. This feature is implemented in the method
def dv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! The confusing part was having
dv
in the test name but that doesn't match the implementation name. Like I mentioned in the previous comment just now, I think it's best to have different methods that cover all of those scenarios. In this case, this method could test the dv behavior, and the name is okay. But other one that tests the entire functionality doesn't need to worry about the implementation details such as the dv number. Does that make sense?