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.

Sign up for GitHub

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

Jump to bottom

Add ChileRut.full_formatted_rut #2460

Merged
merged 7 commits into from Aug 19, 2022
Merged

Add ChileRut.full_formatted_rut #2460

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c22cc2
Add full_rut_with_dots, a method that generates a rut with thousands …
Feb 5, 2022
dd851a2
changed Faker::ChileRut.full_rut_with_dots to Faker::ChileRut.full_fo…
KarlHeitmann Mar 13, 2022
2bfb3e0
add docs for full_rut and full_formatted_rut
vbrazo Aug 6, 2022
2e18995
remote dots
vbrazo Aug 6, 2022
a10d535
changed 1.9.2 to next faker.version on chile_rut.rb
KarlHeitmann Aug 8, 2022
9db24fb
full_formatted_rut merged into full_rut with param formatted
KarlHeitmann Aug 18, 2022
c62c5b7
tag #full_rut with @faker.version next
Zeragamba Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions doc/default/chile_rut.md
View file
Open in desktop
Expand Up @@ -19,4 +19,18 @@ Faker::ChileRut.dv #=> "k"
# check_digit is an alias for dv, for English speaking devs
Faker::ChileRut.rut #=> 30528772
Faker::ChileRut.check_digit #=> "5"

# Returns full rut
# Keyword arguments: min_rut
# Keyword arguments: fixed
Faker::ChileRut.full_rut #=> "30686957-4"
Faker::ChileRut.full_rut(min_rut: 20890156) #=> "30686957-4"
Faker::ChileRut.full_rut(min_rut: 30686957, fixed: true) #=> "30686957-4"

# Returns full formatted rut
# Keyword arguments: min_rut
# Keyword arguments: fixed
Faker::ChileRut.full_formatted_rut #=> "30.686.957-4"
Faker::ChileRut.full_formatted_rut(min_rut: 20890156) #=> "30.686.957-4"
Faker::ChileRut.full_formatted_rut(min_rut: 30686957, fixed: true) #=> "30.686.957-4"
```
23 changes: 23 additions & 0 deletions lib/faker/default/chile_rut.rb
View file
Open in desktop
Expand Up @@ -92,6 +92,29 @@ def full_rut(legacy_min_rut = NOT_GIVEN, legacy_fixed = NOT_GIVEN, min_rut: 0, f
"#{rut(min_rut: min_rut, fixed: fixed)}-#{dv}"
end

##
# Produces a random Chilean RUT (Rol Unico Tributario, ID with 8 digits) with a dv (digito verificador, check-digit).
# with character passed in argument as separator.
#
# @param min_rut [Integer] Specifies the minimum value of the rut.
# @param fixed [Boolean] Determines if the rut is fixed (returns the min_rut value).
# @return [String]
#
# @example
# Faker::ChileRut.full_rut_with_dots #=> "30.686.957-4"
# Faker::ChileRut.full_rut_with_dots(min_rut: 20890156) #=> "30.686.957-4"
# 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)
Copy link
Contributor

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.:

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.

Copy link
Contributor Author

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 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?

Copy link
Contributor

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:

  • happy and unhappy cases for formatted rut
  • fixed true and false cases

Copy link
Contributor

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 well

Copy link
Contributor

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!

Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

warn_for_deprecated_arguments do |keywords|
keywords << :min_rut if legacy_min_rut != NOT_GIVEN
keywords << :fixed if legacy_fixed != NOT_GIVEN
end

"#{rut(min_rut: min_rut, fixed: fixed).to_s.reverse.gsub(/(\d{3})(?=\d)/, '\\1.').reverse}-#{dv}"
end

attr_reader :last_rut
end
end
Expand Down
5 changes: 5 additions & 0 deletions test/faker/default/test_faker_chile_rut.rb
View file
Open in desktop
Expand Up @@ -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
Copy link
Contributor

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 has dv in the name when the implementation doesn't.

Copy link
Contributor Author

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

Copy link
Contributor

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?

assert @tester.full_formatted_rut(min_rut: 30_686_957, fixed: true).split('-')[0] == '30.686.957'
assert @tester.dv == '4'
end
end