Add Faker::Games namespace #1412
Conversation
There was a problem hiding this comment.
Thanks for contributing and adding the Games namespace 👍
Couple of things:
- don't remove the files yet. The old methods should call the new methods, otherwise you'll break users' code in a way that they'll get pissed off.
- you need to deprecate the methods that you're removing. You should do that in the old objects.
I don't know when we'll release these namespaces. I've also worked on the Lorem namespacing recently. Thanks for coming here and submitting this PR. Someone had to get his/her hands dirty 👍 and this PR is awesome!
ps: Please fix the rubocop offense.
README.md
Outdated
| - [Faker::Games::Zelda](doc/zelda.md) | ||
| - [Faker::Gender](doc/gender.md) | ||
| - [Faker::Pokemon](doc/pokemon.md) | ||
| - [Faker::GratefulDead](doc/grateful_dead.md) |
There was a problem hiding this comment.
The order is wrong here. Did you miss the namespace?
|
@ChaoticBoredom we started the discussion in this issue #1318. Please check it out. |
|
Cool, I likely screwed something up in my merge conflict resolution. Will address. I was looking at that issue for guidance, didn't notice it had the text file. I'd left the |
|
@ChaoticBoredom I think the Witcher series has the most cultural impact as a game right now, so I'd put it in |
|
Have addressed most requested changes, still need to add tests around the deprecated methods. |
|
@vbrazo I think I've addressed everything. I did try and use both existing |
|
Yeah. It's a pretty big one. No need to apologize. As long as your changes add the new namespace and help us transform the library in a more useful work tool, we're happy to review and request changes when necessary. I'll pull the code, read and test the changes during the weekend 👍 Thanks for adding the tests for the old methods and for the new namespace. That's necessary. Otherwise we don't know if they're really working. |
| Faker::Games::Dota.quote #=> "Easy now, this stuff is explosive!" | ||
| Faker::Games::Dota.quote(hero = 'alchemist') #=> "Better living through alchemy!" |
There was a problem hiding this comment.
Could we convert this parameter to a keyword argument as it was suggested in this issue #603?
There was a problem hiding this comment.
Could that be done as a follow-up? Keep this PR as more of a strict re-org?
* Move games files into 'games' folder * Migrate all 'elder_scrolls' code to the 'games' namespace * Move 'zelda' to the correct 'namespace' * Update 'fallout' to be in the 'games' namespace * Update 'pokemon' to be in the 'games' namespace * Update 'world_of_warcraft' to be in the 'games' namespace * Update 'overwatch' to be in the 'games' namespace * Update 'myst' to be in the 'games' namespace * Update 'league_of_legends' to be in the 'games' namespace * Update 'dota' to be in the 'games' namespace * Move 'witcher' to be under 'games' namespace * Remove rubocop warning * Restore old file, deprecate methods in favour of new namespaced version * Restore 'dota' file, add deprecations * Restore 'elder_scrolls' and deprecate * Restore 'heroes_of_the_storm' and deprecate * Restore 'league_of_legends' and deprecate * Restore 'myst' and deprecate * Restore 'overwatch' and deprecate * Restore 'pokemon' and deprecate * Restore 'world_of_warcraft' and deprecate * Restore 'zelda' and deprecate * Address mucked up merge * Restore 'fallout' and deprecate * Merge mess still * Address rubocop violation * Remove duplicated entry * Remove extra '::' * Add tests for the newly deprecated methods * Update CHANGELOG.md
Not sure if this is a valuable PR, but this is my attempt to push all the Games into the single folder.
All the Games stuff is now namespaced under
Games.If needed, I can break these up into a bunch of smaller PRs, and I think in the future, a number of smaller PRs would be easier to review. Also likely this should be a minor version bump.
This PR is related to this issue #1318.