-
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
Refactor Faker::IDNumber
to Faker::IdNumber
to be more consistent with other generator's naming convention.
#2858
Refactor Faker::IDNumber
to Faker::IdNumber
to be more consistent with other generator's naming convention.
#2858
Conversation
Faker::IDNumber
to Faker::IdNumber
to be more consistent with other generating naming convention.
Faker::IDNumber
to Faker::IdNumber
to be more consistent with other generating naming convention.Faker::IDNumber
to Faker::IdNumber
to be more consistent with other generator's naming convention.
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 @Jamal-A-Mohamed and Salvador! This is great. I left one suggestion, let me know if you have any questions. Thanks!
The Test failure is fixed by #2868 could you rebase this branch to get the changes there? |
29483a4
to
fdc7d4b
Compare
lib/helpers/deprecated_class.rb
Outdated
end | ||
end | ||
|
||
class DeprecatedClass |
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.
What do you think of naming this Deprecator
? I find it a better name because class is not adding too much to the name.
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'm thinking if we can do something like how ActiveSupport does but a simplified version for deprecating generators: https://github.com/rails/rails/blob/main/activesupport/lib/active_support/deprecation/constant_accessor.rb
It would be a module mixin that would add the const_missing
and intercept calls to the deprecated module. The other mixin method would let you register the deprecated and the new modules.
If we could this in a module to be included, we can just add it to the generator to deprecate_constant
passing the generator and the replacement, ie:
# id_number.rb
module Faker
class IdNumber < Base
include Faker::Deprecator
deprecate_constant(Faker::IDNumber, Faker::IdNumber)
end
end
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.
Yeah, I think this is a good idea, will implement it that way.
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.
Cool, thank you! That would fit nicely with the others to be deprecated. Let us know if you need any help.
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.
Hi @stefannibrasil ,
Sorry for the taking so long on this but haven't had a chance to touch this till this morning.
I am not able to figure out a way to intercept the const_missing calls to the Faker module itself. i.e Faker::Constant_name. I tried to prepend the extension/module to the Faker module itself but that doesn't seem to work. will continue to look at it but interested to hear if you have way to get around that.
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.
Hi @Jamal-A-Mohamed thank you for your patience! I was almost there and @thdaraujo helped me with the final details. Here's how we could make it work: https://github.com/faker-ruby/faker/compare/main...hexdevs:faker:deprecator?expand=1
Not sure if you can cherry-pick this commit 8f54cbe to this branch.
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.
Awesome! Great Job to you both.
I cherry picked that from your guy's commit and pushed it up.
I would need to re-read to see what I was missing.
Let me know if you want me to squash commits and or write more tests.
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.
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.
Done!
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.
Nice, thank you for helping out on this one! 🎉
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.
🎉
… with other generator's naming convention. (faker-ruby#2858) * Deprecate IDNumber to IdNumber per issue#2787 * wip * Add a Faker::Deprecator module Co-authored-by: Thiago Araujo <[email protected]> * Add more specs and improve warning message --------- Co-authored-by: jamal.mohamed <[email protected]> Co-authored-by: Stefanni Brasil <[email protected]> Co-authored-by: Thiago Araujo <[email protected]>
* update Faker::Australia to Faker::Locations::Australia * Bump minitest from 5.21.1 to 5.21.2 (#2894) Bumps [minitest](https://github.com/minitest/minitest) from 5.21.1 to 5.21.2. - [Changelog](https://github.com/minitest/minitest/blob/master/History.rdoc) - [Commits](minitest/minitest@v5.21.1...v5.21.2) --- updated-dependencies: - dependency-name: minitest dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump rubocop from 1.59.0 to 1.60.2 (#2896) Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.59.0 to 1.60.2. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.59.0...v1.60.2) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add Kenya to supported countries (#2871) * Add Kenya to supported countries * Add tests for locale * clan up pull request * Add landline telephone numbers and cell phone formats * Bump minitest from 5.21.2 to 5.22.2 (#2902) Bumps [minitest](https://github.com/minitest/minitest) from 5.21.2 to 5.22.2. - [Changelog](https://github.com/minitest/minitest/blob/master/History.rdoc) - [Commits](minitest/minitest@v5.21.2...v5.22.2) --- updated-dependencies: - dependency-name: minitest dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump test-unit from 3.6.1 to 3.6.2 (#2906) Bumps [test-unit](https://github.com/test-unit/test-unit) from 3.6.1 to 3.6.2. - [Release notes](https://github.com/test-unit/test-unit/releases) - [Commits](test-unit/test-unit@3.6.1...3.6.2) --- updated-dependencies: - dependency-name: test-unit dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Refactor `Faker::IDNumber` to `Faker::IdNumber` to be more consistent with other generator's naming convention. (#2858) * Deprecate IDNumber to IdNumber per issue#2787 * wip * Add a Faker::Deprecator module Co-authored-by: Thiago Araujo <[email protected]> * Add more specs and improve warning message --------- Co-authored-by: jamal.mohamed <[email protected]> Co-authored-by: Stefanni Brasil <[email protected]> Co-authored-by: Thiago Araujo <[email protected]> * Fix/Deprecate FmaBrotherhood Generator (#2856) * Favor 'The Room' instead of 'Room' This commit will rename instances of room to the_room including the locales. Originally the `room.md` had a typo that referenced ::Room and not ::TheRoom which is also fixed here. Ref: - #2787 Co-authored-by: Jamal-A-Mohamed <[email protected]> Co-authored-by: Salvador <[email protected]> * Fix/Deprecate FmaBrotherhood Generator This commit fixes the naming discrpencies with the FmaBrotherhood (now FullmetalAlchemistBrotherhood) class and its filename. This adds deprecation warnings for the old FmaBrotherhood class and also makes the new FullmetalAlchemistBrotherhood class. Fix: - #2853 * Refactor deprecation for `FmaBrotherhood` This commit will refactor the changes from `FmaBrotherhood` to `FullmetalAlchemistBrotherhood` and use `Faker::Deprecator`. --------- Co-authored-by: Jamal-A-Mohamed <[email protected]> Co-authored-by: Salvador <[email protected]> Co-authored-by: Stefanni Brasil <[email protected]> * Bump i18n from 1.14.1 to 1.14.4 (#2913) Bumps [i18n](https://github.com/ruby-i18n/i18n) from 1.14.1 to 1.14.4. - [Release notes](https://github.com/ruby-i18n/i18n/releases) - [Changelog](https://github.com/ruby-i18n/i18n/blob/master/CHANGELOG.md) - [Commits](ruby-i18n/i18n@v1.14.1...v1.14.4) --- updated-dependencies: - dependency-name: i18n dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove duplicates in doc file (#2914) * Bump rubocop from 1.60.2 to 1.62.1 (#2916) Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.60.2 to 1.62.1. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.60.2...v1.62.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump yard from 0.9.34 to 0.9.36 (#2909) Bumps [yard](https://github.com/lsegal/yard) from 0.9.34 to 0.9.36. - [Release notes](https://github.com/lsegal/yard/releases) - [Changelog](https://github.com/lsegal/yard/blob/main/CHANGELOG.md) - [Commits](lsegal/yard@v0.9.34...v0.9.36) --- updated-dependencies: - dependency-name: yard dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Deprecated Faker::Australia * Added Docs for Australia * Updated Readme to include Locations * Updated locales path for australia * updated test to differentiate deprecated methods * Removed whitespaces from australia.yml --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Andrew Nduati <[email protected]> Co-authored-by: Jamal-A-Mohamed <[email protected]> Co-authored-by: jamal.mohamed <[email protected]> Co-authored-by: Stefanni Brasil <[email protected]> Co-authored-by: Thiago Araujo <[email protected]> Co-authored-by: Kirk Wang <[email protected]> Co-authored-by: Jamal-A-Mohamed <[email protected]> Co-authored-by: Salvador <[email protected]> Co-authored-by: Michael Marusyk <[email protected]>
Co-authored-by: Salvador Tena [email protected]
Motivation / Background
This Pull Request has been created because of this issue #2787 which raises the inconsistency of generator names in the codebase. The PR changes the class name from IDNumber to IdNumber which puts it in line with the rest of the generators and updates tests and documentation as well.
Additional information
Testing:
Before changing the test files I ran the test task and made sure the delegation worked properly and that deprecation warnings were outputting correctly. screenshot is below. added the calling from the message after the fact.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
If you're proposing a new generator: