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

Added Faker::Company.indian_gst_number fixed #2823 #2825

Merged
merged 8 commits into from Nov 2, 2023

Conversation

ankitkhadria
Copy link
Contributor

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:

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

If you're proposing a new generator:

  • Open an issue first for discussion before you write any code.
  • Double-check the existing generators documentation to make sure the new generator you want to add doesn't already exist.
  • You've reviewed and followed the Documentation guidelines.

@stefannibrasil stefannibrasil linked an issue Oct 7, 2023 that may be closed by this pull request
Copy link
Contributor

@stefannibrasil stefannibrasil left a 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 🌟

lib/faker/default/company.rb Show resolved Hide resolved
test/faker/default/test_faker_company.rb Show resolved Hide resolved
test/faker/default/test_faker_company.rb Outdated Show resolved Hide resolved
@ankitkhadria
Copy link
Contributor Author

Thank you so much, @stefannibrasil, for reviewing it.

Comment on lines 481 to 482
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')
Copy link
Contributor

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?

Copy link
Contributor Author

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>
Copy link
Contributor

@stefannibrasil stefannibrasil left a comment

Choose a reason for hiding this comment

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

🎉

@stefannibrasil stefannibrasil merged commit 205bc52 into faker-ruby:main Nov 2, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New generator request: Faker::Company.indian_gst_number
2 participants