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

Rename 'Show' group to 'Entertainment' and add new provider #647

Merged
merged 1 commit into from Jan 30, 2023

Conversation

panilya
Copy link
Collaborator

@panilya panilya commented Jan 23, 2023

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2023

Codecov Report

Merging #647 (4617712) into main (402b161) will decrease coverage by 0.11%.
The diff coverage is 47.36%.

@@             Coverage Diff              @@
##               main     #647      +/-   ##
============================================
- Coverage     92.77%   92.67%   -0.11%     
- Complexity     2623     2627       +4     
============================================
  Files           281      282       +1     
  Lines          5396     5403       +7     
  Branches        589      589              
============================================
+ Hits           5006     5007       +1     
- Misses          240      244       +4     
- Partials        150      152       +2     
Impacted Files Coverage Δ
src/main/java/net/datafaker/Faker.java 66.66% <ø> (ø)
...r/providers/entertainment/AquaTeenHungerForce.java 100.00% <ø> (ø)
.../net/datafaker/providers/entertainment/Avatar.java 100.00% <ø> (ø)
...et/datafaker/providers/entertainment/Babylon5.java 100.00% <ø> (ø)
...faker/providers/entertainment/BackToTheFuture.java 100.00% <ø> (ø)
...tafaker/providers/entertainment/BigBangTheory.java 100.00% <ø> (ø)
...afaker/providers/entertainment/BojackHorseman.java 100.00% <ø> (ø)
...t/datafaker/providers/entertainment/BossaNova.java 100.00% <ø> (ø)
...datafaker/providers/entertainment/BreakingBad.java 100.00% <ø> (ø)
...aker/providers/entertainment/BrooklynNineNine.java 100.00% <ø> (ø)
... and 67 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@snuyanzin
Copy link
Collaborator

Why do we need to rename it the third time?

@panilya
Copy link
Collaborator Author

panilya commented Jan 23, 2023

Because of the books, there's no sense in adding a books group and a show group is not suitable for books, so I renamed it to entertainment

@snuyanzin
Copy link
Collaborator

snuyanzin commented Jan 23, 2023

i would rather follow similar category structure and naming as for https://github.com/faker-ruby/faker
to make it easier for users to switch to datafaker

@panilya
Copy link
Collaborator Author

panilya commented Jan 23, 2023

Yeah, but a lot of books have movie interpretations, in this case, where should that provider belong?

@snuyanzin
Copy link
Collaborator

if a book appeared first then it should be in books
if a movie appeared first then it should be in movies and so on

it looks other faker follow the similar rule

@bodiam
Copy link
Contributor

bodiam commented Jan 24, 2023

i would rather follow similar category structure and naming as for https://github.com/faker-ruby/faker
to make it easier for users to switch to datafaker

I don't think the ruby fake makes much sense, and nobody will switch from Ruby to Java. Besides, the package names are mostly hidden from our users anyway, so I don't see much of an issue here.

@snuyanzin
Copy link
Collaborator

snuyanzin commented Jan 24, 2023

the issue is that initially it was called Movies, then it was renamed to Show now to Entertaiment...
Probably we should stop at some point with just renaming same package... otherwise it's becoming confusing

Besides, the package names are mostly hidden from our users anyway, so I don't see much of an issue here.

In fact there is not only package name, also an interface name.
It means each renaming means NON compilable code for users who somehow should realise what is a new name for their interface MovieProviders or ShowProviders in case they use it

@bodiam
Copy link
Contributor

bodiam commented Jan 24, 2023

I don't doubt we have to do a few more renames when we move some of the base providers to the right package. For example, we have providers related to an author. An entertainment package would be okay for that maybe, but not movies nor shows.

@snuyanzin
Copy link
Collaborator

An entertainment package would be okay for that maybe, but not movies nor shows.

With Entertainement there is an issue of overlapping with existing VideoGames.
That's why it's better to extract some sub categories into a separate one (like suggested above, ok, could be different from rube faker)

@bodiam
Copy link
Contributor

bodiam commented Jan 24, 2023

With Entertainement there is an issue of overlapping with existing VideoGames. That's why it's better to extract some sub categories into a separate one (like suggested above, ok, could be different from rube faker)

Fair enough, but we could also consider moving videogames to entertainment. Not sure if it deserves its own specific package.

@snuyanzin
Copy link
Collaborator

snuyanzin commented Jan 24, 2023

I would rather continue splitting providers than merging them.
Ideally any group of 5-10 and more providers should have it's own name.
For those who cares it would be a good chance to construct their own faker.
For those who doesn't it's still possible to use just Faker class

@panilya
Copy link
Collaborator Author

panilya commented Jan 26, 2023

For me, there are two ways:

  1. Entertainment package for books, games, movies, shows, and everything related to entertainment.
  2. Separate packages for each entertainment type. videogame for games, tvshow for shows, book for books, etc..
    And in the second case, it also makes sense to separate base package into smaller ones

@snuyanzin
Copy link
Collaborator

Separate packages for each entertainment type. videogame for games, tvshow for shows, book for books, etc..
And in the second case, it also makes sense to separate base package into smaller ones

yes it would be a better approach. However it does not make a big sense to extract something in a group of less than 5 items or something like that

@bodiam
Copy link
Contributor

bodiam commented Jan 28, 2023

The idea was to one day split the faker into multiple modules. I think it would be a bit silly to have a module with 5 authors, though we could always bundle multiple packages into 1 module.

So, I think it doesn't really matter how we organize things. It doesn't affect the API, so I'm fine either way, as in, faker-enterfainment could contain books, tv shows, games, movies, etc, while faker-core/ base contains some number, boolean and perhaps person data.

On the other hand, I'm not really sure what problem we're solving by splitting things, I've never heard of a real problem the way the faker is currently set up.

@kingthorin
Copy link
Collaborator

If the project follows semver then a change such as this is breaking which would make the next release 2.0.0. (Since users would have to adapt their code to new reorganized providers.)

@bodiam
Copy link
Contributor

bodiam commented Jan 30, 2023

I don't see that. There is no import of these packages, the recommended way to use the faker is :

faker.provider.property, such as:

faker.address().streetName()

As such, it doesn't matter in which package the address provider is, and I don't consider this a real api change. Happy to hear why this would be a breaking change though.

@kingthorin
Copy link
Collaborator

You’re right, I was originally thinking this was changing faker.show() to become faker.entertainment().

@bodiam
Copy link
Contributor

bodiam commented Jan 30, 2023

@kingthorin Ah, good point! But no, this won't change in this case. If it did, we probably wouldn't go this route, but we would at least first deprecate the show() method, and then remove it or so. We haven't been fully following semver, we would have been at Datafaker 9 by now maybe if we followed semver to the letter, but we aim to keep the breaking changes to a minimum.

@bodiam bodiam merged commit 3457e4e into datafaker-net:main Jan 30, 2023
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.

None yet

5 participants