-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Added Faker::Company.indian_gst_number fixed #2823 #2825
Added Faker::Company.indian_gst_number fixed #2823 #2825
Conversation
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.
Thank you, @ankitkhadria I left some comments/suggestions 🌟
Thank you so much, @stefannibrasil, for reviewing it. |
lib/faker/default/company.rb
Outdated
state_code_ranges = ['02'..'38', ['98']] | ||
raise ArgumentError, 'state code must be in a range of 02 to 38 or 98' if state_code && (!state_code_ranges[0].cover?(state_code) || state_code_ranges[1][0] != '98') |
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 tested this locally and the generator was accepting any argument. For example:
Faker::Company.indian_gst_number(state_code: '99') = > "99BBHPN5079J6ZN"
Faker::Company.indian_gst_number(state_code: '100') => "100IXDHY2033Z1ZL"
I believe this is what we want:
state_code_ranges = ['02'..'38', ['98']]
if state_code && !(state_code_ranges[0].cover?(state_code) || state_code == '98')
raise ArgumentError, 'state code must be in a range of 02 to 38 or 98'
end
It was a bit hard to follow the condition, so this way is easier to understand what we want. What do you think?
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.
You're right, the generator was accepting invalid state codes too, it seems like cover?
method is related to the Comparable module, and checks whether an item would fit between the end points in a sorted list. It will return true even if the item is not in the set implied by the Range, so using include?
to check whether the item is in complete set.
2.7.4 :002 > ("02".."38").cover?("100")
=> true
2.7.4 :003 > ("02".."38").cover?("1000")
=> true
2.7.4 :004 > ("02".."38").include?("100")
=> false
changed to
raise ArgumentError, 'state code must be in a range of 02 to 38 or 98' if state_code && !(state_code_ranges[0].include?(state_code) || state_code == '98')
Co-authored-by: Stefanni Brasil <stefannibrasil@gmail.com>
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.
🎉
Motivation / Background
This PR addresses a specific need for businesses in India. In India, every company is required to have a valid Goods and Service Tax (GST) number. The motivation behind this PR is to enhance the Faker gem's functionality by introducing a new feature that generates random, valid GST numbers for companies.
This addition to the Faker gem is important for several reasons:
It helps developers working on projects related to Indian businesses and taxation to generate realistic test data.
It streamlines the process of creating test environments that closely resemble real-world scenarios for companies operating in India.
Additional information
A new generator for generating random valid GST numbers for Indian companies has been added to the Fake::Company module.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
If you're proposing a new generator: