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

Add max_rut option to Faker::ChileRut.rut #2778

Merged
merged 3 commits into from Jul 15, 2023
Merged

Conversation

hacktivista
Copy link
Contributor

Motivation / Background

Allows to set a maximum RUT number for ChileRut. I needed this because juridical person RUTs start at a higher number than natural person RUTs. I needed to create human RUTs.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug, refactor something, or add a feature.
  • Tests and Rubocop are passing before submitting your proposed changes.
    (test_brazilian_id is failing, but that's not related to my code)

Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I left some questions as I need more context to be able to review your PR.

I think this generator is a good candidate for a refactor: we've just introduced the PositionalGenerator as an alternative for generating complex values.

Maybe it could make this code a bit easier to read. Let me know if you'd be interested in that! (Or the refactor could also happen in a future PR, no need to do it here).

lib/faker/default/chile_rut.rb Show resolved Hide resolved
lib/faker/default/chile_rut.rb Outdated Show resolved Hide resolved
test/faker/default/test_faker_chile_rut.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

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

cool, LGTM!

@thdaraujo thdaraujo changed the title Allow max_rut option on ChileRut Add max_rut option to Faker::ChileRut.rut Jul 15, 2023
@thdaraujo thdaraujo merged commit 56fc3c8 into faker-ruby:main Jul 15, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants