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.

Sign up for GitHub

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

Jump to bottom

Fix/Deprecate FmaBrotherhood Generator #2856

Merged
merged 7 commits into from
Feb 26, 2024
Merged

Fix/Deprecate FmaBrotherhood Generator #2856

merged 7 commits into from
Feb 26, 2024

Conversation

kirkkwang
Copy link
Contributor

Motivation / Background

This Pull Request has been created because of this issue:

Additional information

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.

kirkkwang and others added 2 commits November 13, 2023 14:23
@kirkkwang @Jamal-A-Mohamed @Chava888
Favor 'The Room' instead of 'Room'
9ea7a9c 
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:
  - faker-ruby#2787

Co-authored-by: Jamal-A-Mohamed <[email protected]>
Co-authored-by: Salvador <[email protected]>
@kirkkwang
Merge branch 'faker-ruby:main' into main
1c8e604
@kirkkwang
Fix/Deprecate FmaBrotherhood Generator
3fef6b3 
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:
  - faker-ruby#2853
stefannibrasil
stefannibrasil reviewed Dec 8, 2023
View reviewed changes
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 @kirkkwang for working on this and for your patience for this review. I left a few comments, let me know if you have any questions :)

lib/faker/japanese_media/fullmetal_alchemist_brotherhood.rb Outdated Show resolved Hide resolved
lib/faker/japanese_media/fullmetal_alchemist_brotherhood.rb Show resolved Hide resolved
test/faker/japanese_media/test_faker_fullmetal_alchemist_brotherhood.rb Outdated Show resolved Hide resolved
@stefannibrasil
Copy link
Contributor

hi @kirkkwang just checking on this one :)

@kirkkwang
Copy link
Contributor Author

hi @kirkkwang just checking on this one :)

Oh thanks for reminding me! I totally forgot 😅

@stefannibrasil
Copy link
Contributor

Hi @kirkkwang we merged this Deprecator that we can use for this one as well: #2858

@kirkkwang
Copy link
Contributor Author

@stefannibrasil wow that makes this so much easier, great addition! I've pushed up a change with a proposed refactor, lemme know if that fits the idea. If not I can revert.

stefannibrasil
stefannibrasil reviewed Feb 26, 2024
View reviewed changes
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 @kirkkwang ! Left two small suggestions.

test/faker/japanese_media/test_faker_fullmetal_alchemist_brotherhood.rb Outdated Show resolved Hide resolved
test/test_faker_deprecator.rb Outdated Show resolved Hide resolved
stefannibrasil
stefannibrasil reviewed Feb 26, 2024
View reviewed changes
test/faker/japanese_media/test_faker_fullmetal_alchemist_brotherhood.rb Outdated
@@ -18,4 +18,22 @@ def test_city
def test_country
assert_match(/\w+/, @tester.country)
end

def test_using_a_deprecated_generator_returns_a_warning_message
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost there! Sorry, I wasn't super clear. For this test, we want to keep both classes covered.

We can have the old test in this same file, like we did for IDNumber:

https://github.com/faker-ruby/faker/blob/main/test/faker/default/test_faker_id_number.rb#L292

So we'd have two classes in this file. One for testing Faker::JapaneseMedia::FmaBrotherhood and another for Faker::JapaneseMedia::FullmetalAlchemistBrotherhood. Does that make sense?

We want to be sure the behaviour is maintained despite of which class is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense, do i keep this test still in here?
test_using_a_deprecated_generator_returns_a_warning_message

Copy link
Contributor

Choose a reason for hiding this comment

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

That behaviour is covered by the deprecator test, so we can remove them from here 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefannibrasil i pushed the changes, let me know if this is what you're looking for

@kirkkwang
Refactor deprecation for FmaBrotherhood
50b540f 
This commit will refactor the changes from `FmaBrotherhood` to
`FullmetalAlchemistBrotherhood` and use `Faker::Deprecator`.
stefannibrasil
stefannibrasil approved these changes Feb 26, 2024
View reviewed changes
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, @kirkkwang 🎉

lib/faker/japanese_media/fullmetal_alchemist_brotherhood.rb
end
end
end

include Faker::Deprecator
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

@stefannibrasil stefannibrasil merged commit 7990a43 into faker-ruby:main Feb 26, 2024
8 checks passed
keshavbiswa pushed a commit to keshavbiswa/faker that referenced this pull request Mar 17, 2024
@kirkkwang @Jamal-A-Mohamed
@Chava888 @keshavbiswa
Fix/Deprecate FmaBrotherhood Generator (faker-ruby#2856)
964a223 
* 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:
  - faker-ruby#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:
  - faker-ruby#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]>
stefannibrasil pushed a commit that referenced this pull request Mar 19, 2024
@keshavbiswa @dependabot
@AndrewNduati @Jamal-A-Mohamed @thdaraujo @kirkkwang @Chava888 @mmarusyk
Update Faker::Australia to Faker::Locations::Australia (#2869)
fcf41f1 
* 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]>
@stefannibrasil stefannibrasil mentioned this pull request Mar 26, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefannibrasil stefannibrasil stefannibrasil approved these changes

Assignees
No one assigned
Labels
⏲️ PR: In Queue for Review rubyconf2023
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kirkkwang @stefannibrasil @thdaraujo