Add ChileRut.full_formatted_rut #2460
Conversation
…separated by dots
…rmatted_rut, a better semantic name
vbrazo
changed the title
lib/faker/default/chile_rut.rb
Outdated
| # Faker::ChileRut.full_rut_with_dots(min_rut: 30686957, fixed: true) #=> "30.686.957-4" | ||
| # | ||
| # @faker.version next | ||
| def full_formatted_rut(legacy_min_rut = NOT_GIVEN, legacy_fixed = NOT_GIVEN, min_rut: 0, fixed: false) |
There was a problem hiding this comment.
What do you think of following the same pattern we use when generating ID Numbers? i.e.:
def brazilian_citizen_number(legacy_formatted = NOT_GIVEN, formatted: false)
...
end
That way we could simply have a formatted: keyword in the keyword full_rut method.
There was a problem hiding this comment.
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 the formatted: 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.
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:
- happy and unhappy cases for formatted rut
fixedtrue and false cases
| @@ -24,4 +24,9 @@ def test_check_digit | |||
| assert @tester.rut(min_rut: 30_686_957, fixed: true) == 30_686_957 | |||
| assert @tester.dv == '4' | |||
| end | |||
|
|
|||
| def test_full_formatted_rut_has_dv | |||
There was a problem hiding this comment.
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 has dv in the name when the implementation doesn't.
There was a problem hiding this comment.
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.
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?
lib/faker/default/chile_rut.rb
Outdated
| # Faker::ChileRut.full_rut_with_dots(min_rut: 30686957, fixed: true) #=> "30.686.957-4" | ||
| # | ||
| # @faker.version next | ||
| def full_formatted_rut(legacy_min_rut = NOT_GIVEN, legacy_fixed = NOT_GIVEN, min_rut: 0, fixed: false) |
There was a problem hiding this comment.
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 well
There was a problem hiding this comment.
I forgot to ask that on my review, thanks for the comment @Zeragamba!
There was a problem hiding this comment.
Hi! I don't quite understand, is this related with your first commit here ?
There was a problem hiding this comment.
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.
|
Hi @stefannibrasil , thanks for your review. I left some comments in your observations :) |
thank you, Karl! I am going on vacation for a few days so I might take longer to reply. Let me know if my comments are helpful. |
There was a problem hiding this comment.
Leaving my review as comment to avoid blocking this PR for being merged. @Zeragamba I'm going on vacation so I will leave this one for you to approve and merge once the changes are addressed. Thanks :)
stefannibrasil
dismissed
their stale review
Leaving my review as comment to avoid blocking this PR for being merged. @Zeragamba I'm going on vacation so I will leave this one for @Zeragamba to approve and merge. Thanks :)
KarlHeitmann
force-pushed
the
new_method_full_rut_with_dots_added_to_chile_rut
branch
from
febbeb7
to
9db24fb
Compare
since we're making a change to an existing api, we need to update the version the api is available in.
No-StoryDescription:
In Chile, we have a RUT, kind of an ssn. The module written by oxfist was very nice and useful. But sometimes in Chile, we are asked to type RUT with a dot that separates the thousand and millions.
I've added the method
.full_formatted_rutwhich will generate a RUT string with its validator digit (calleddvon the code) and every three numbers it will have a dot separator. The example I'll write here is the same as it is written on the yard documentation of the new method.Anything you need I'll keep an eye on this PR.
Best regards,
Karl