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

Organise fakers into categories #1418

Closed
wants to merge 1 commit into from
Closed

Organise fakers into categories #1418

wants to merge 1 commit into from

Conversation

boardfish
Copy link
Contributor

@boardfish boardfish commented Oct 17, 2018

Right now, these categories are somewhat minimal and could be expanded. For example, something like Faker::Movies::HarryPotter should arguably be in some Books module instead as the book series has greater influence than the movies. However, this adds some consistency overall that I felt necessary.

I'm aware that two tests are failing but I haven't been able to pinpoint why that is. If a maintainer is able to pull my fork and assist with this, I'd very much appreciate it.

I intend to follow up this PR at some point to fix some pre-existing spelling/grammar issues across the project, most noticeable with flaws such as stranger_thing.rb, but I figured they'd be outside the scope of this one.

This PR is related to this issue #1318.

Right now, these categories are somewhat minimal and could be expanded.
For example, something like `Faker::Movies::HarryPotter` should arguably
be in some `Books` module instead as the book series has greater
influence than the movies. However, this adds some consistency overall
that I felt necessary.
Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR 👍

I see that you're adding a few namespaces and that's pretty helpful. We started the discussion on this issue and we still have a lot of work to do.

I have a few considerations:

  • You're adding namespaces for Games, but we already have a PR for the Games namespace that covers all the objects. So I'd suggest removing the Games changes. If you want to collaborate, take a look at the PR, read the changes and let the contributor know your considerations.
  • I saw that you also created namespaces for movies, tv, anime, comics. I'd prefer to have these namespaces in separate PRs. The title of the PRs should be something like:
    Add Faker::Movies namespace
    Add Faker::Tv namespace
    Add Faker::Comics namespace
    Add Faker::Anime namespace
  • What's the difference between tv and movies? tv is for TvShows?
  • If you're removing a method, you need to deprecate the old method. So I'd advice you not to delete the old object and deprecate its methods. Otherwise when we release the PR, users will start having crashes in their systems. You can find an example here.

@boardfish
Copy link
Contributor Author

Gotcha. I think I went a little bit ham on this one — the inconsistency on namespaces really stuck out to me, so it'd be good if PRs relating to this can be merged soon. I'll close this one, but I'll likely make a few others as you say to break things down more. I'll probably do Anime first as that doesn't encompass much right now but could be likely to grow, as potential for My Hero Academia and similarly expansive media to be included is pretty strong.

With regards to deprecation, what's a good reference on how to handle that in this report? Is there another PR that's done it in a way you'd like to see as the standard?

@boardfish boardfish closed this Oct 17, 2018
@vbrazo
Copy link
Member

vbrazo commented Oct 18, 2018

@boardfish sounds good. You can start with Faker::Anime. You can also discuss namespaces in that open issue #1318. Our current challenge is to define good taxonomy and your suggestions would be pretty awesome. We enjoy discussing with the community to understand what they like and dislike, because at the end of the day they'll use the gem.

Check this PR #1264 out. In this example, we deprecated Faker::Name.title in the version 1.9.1 (current version), because we'd already added Faker::Job.title in a previous PR, and we noticed that we had two methods that were doing the same thing.

In version 1.9.1, Faker::Name.title and Faker::Job.title work, but if you use Faker::Name.title, you should receive a warning message, because in the next version we're going to remove Faker::Name.title. This is good because it avoids crashes and unhappy users coming here to open issues complaining about life. We usually release a new version 2-3 times a year to make sure that everyone is warned.

To deprecate a method, you just need to add extend Gem::Deprecate to the class and add a deprecate method: deprecate :title, 'Faker::Job.title', 2018, 9.

Feel free to contribute by adding namespaces or new objects. We're always happy to see new PRs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants