Add ChileRut.full_formatted_rut #2460

KarlHeitmann · 2022-02-05T03:46:15Z

No-Story

Description:

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_rut which will generate a RUT string with its validator digit (called dv on 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.

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"

Anything you need I'll keep an eye on this PR.

Best regards,

Karl

…separated by dots

lib/faker/default/chile_rut.rb

…rmatted_rut, a better semantic name

lib/faker/default/chile_rut.rb

stefannibrasil

Hi there! Thank you for your contribution 🇨🇱 I left a few suggestions and a question. Let me know what you think.

stefannibrasil · 2022-08-10T19:21:28Z

lib/faker/default/chile_rut.rb

+      #   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)


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.

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?

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

stefannibrasil · 2022-08-10T19:25:03Z

test/faker/default/test_faker_chile_rut.rb

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


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.

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

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?

Zeragamba · 2022-08-11T14:21:38Z

lib/faker/default/chile_rut.rb

+      #   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)


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

I forgot to ask that on my review, thanks for the comment @Zeragamba!

Hi! I don't quite understand, is this related with your first commit here ?

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.

KarlHeitmann · 2022-08-12T02:02:24Z

Hi @stefannibrasil , thanks for your review. I left some comments in your observations :)

stefannibrasil · 2022-08-16T20:53:40Z

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.

stefannibrasil

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

@Zeragamba

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

since we're making a change to an existing api, we need to update the version the api is available in.

Add full_rut_with_dots, a method that generates a rut with thousands …

3c22cc2

…separated by dots

vbrazo force-pushed the master branch from de1e7e1 to ddb6fce Compare March 5, 2022 18:26

vbrazo reviewed Mar 12, 2022

View reviewed changes

lib/faker/default/chile_rut.rb Outdated Show resolved Hide resolved

changed Faker::ChileRut.full_rut_with_dots to Faker::ChileRut.full_fo…

dd851a2

…rmatted_rut, a better semantic name

stefannibrasil added 🙏🏽 PR: Needs Documentation ⭐ PR: Feature labels Aug 5, 2022

vbrazo removed the 🙏🏽 PR: Needs Documentation label Aug 6, 2022

vbrazo added 2 commits August 6, 2022 12:13

add docs for full_rut and full_formatted_rut

2bfb3e0

remote dots

2e18995

vbrazo approved these changes Aug 6, 2022

View reviewed changes

vbrazo changed the title ~~Add full_rut_with_dots to ChileRut~~ Add ChileRut.full_formatted_rut Aug 6, 2022

Zeragamba suggested changes Aug 8, 2022

View reviewed changes

lib/faker/default/chile_rut.rb Outdated Show resolved Hide resolved

Zeragamba assigned vbrazo and Zeragamba Aug 8, 2022

changed 1.9.2 to next faker.version on chile_rut.rb

a10d535

stefannibrasil previously requested changes Aug 10, 2022

View reviewed changes

Zeragamba suggested changes Aug 11, 2022

View reviewed changes

stefannibrasil reviewed Aug 16, 2022

View reviewed changes

full_formatted_rut merged into full_rut with param formatted

9db24fb

KarlHeitmann force-pushed the new_method_full_rut_with_dots_added_to_chile_rut branch from febbeb7 to 9db24fb Compare August 18, 2022 00:47

tag #full_rut with @faker.version next

c62c5b7

since we're making a change to an existing api, we need to update the version the api is available in.

Zeragamba approved these changes Aug 19, 2022

View reviewed changes

Zeragamba merged commit 35a48ec into faker-ruby:master Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ChileRut.full_formatted_rut #2460

Add ChileRut.full_formatted_rut #2460

KarlHeitmann commented Feb 5, 2022 •

edited by vbrazo

Loading

stefannibrasil left a comment

stefannibrasil Aug 10, 2022

KarlHeitmann Aug 12, 2022

stefannibrasil Aug 16, 2022

stefannibrasil Aug 10, 2022 •

edited

Loading

KarlHeitmann Aug 12, 2022

stefannibrasil Aug 16, 2022

Zeragamba Aug 11, 2022

stefannibrasil Aug 11, 2022

KarlHeitmann Aug 12, 2022

stefannibrasil Aug 16, 2022

KarlHeitmann commented Aug 12, 2022

stefannibrasil commented Aug 16, 2022

stefannibrasil left a comment •

edited

Loading

Add ChileRut.full_formatted_rut #2460

Add ChileRut.full_formatted_rut #2460

Conversation

KarlHeitmann commented Feb 5, 2022 • edited by vbrazo Loading

Description:

stefannibrasil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefannibrasil Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlHeitmann commented Aug 12, 2022

stefannibrasil commented Aug 16, 2022

stefannibrasil left a comment • edited Loading

Choose a reason for hiding this comment

KarlHeitmann commented Feb 5, 2022 •

edited by vbrazo

Loading

stefannibrasil Aug 10, 2022 •

edited

Loading

stefannibrasil left a comment •

edited

Loading