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
Add support for multiple locale data generation #1052
Conversation
The changes needed to ensure all tests will pass will most likely require backward-incompatible changes if the The random generator related stuff can probably be proxied in a sane way like seeding the same value across all internal generators, but the others do not make as much sense like We can name this proxy class something else to keep changes simple while still providing multiple locale support. I would like to know your thoughts on this @fcurella. |
Thank you much for looking into this thorny issue @malefice ! I'm ok with breaking compatibility, as long as we document what we're breaking and provide users with documentation on how to migrate their code. One feature I want to be very careful about not breaking is the command-line |
Yeah, it is a very thorny issue! |
Alright. Thanks for the guidance. I will try to think of something more elegant as well. |
The flake8 errors are related to PR #1058. There is another error because Other than that, I think the new Single locale mode
In what I call multiple locale mode, attempting to proxy any Instead, I think we should just leave it to users to subclass and create their own implementation if they really want to. After all, each of the internal As for data generation, multiple locale mode will randomly choose a Multiple locale mode
As for the locale strings I mentioned earlier, both |
Would it still possible to seed the generator in multiple locale mode?
I think it should be consistent with what the constructor does. In other words, it should accept both, and normalize to |
Yes but on an individual
If you really want a default implementation, we can probably proxy Personally, I do not know if that will be a common enough use case, which is why I did not provide a proxy level implementation, and why I think it should be up to the users to subclass for their use cases. |
the problem with called Ideally, we want three ways to seed random values:
|
Actually On that note, the proxy class can have a
So what are your thoughts? |
Thank you for the clarification @malefice . This is how I'm thinking it could work:
|
Yes, that is how
Yeah that is certainly possible, but I suggest raising a Either way, doing this will definitely break any old code that calls Old code that will break
Method 1
Method 2
So are you okay with this? |
That's a much better idea! Let's do a
I'm comfortable with that, I think it's worth it for the additional features we gain and cleaner code. As long as we increase the major version and have clear docs on how to migrate user's code, it should be fine. |
@fcurella can you review my work? once it is okay, i will start writing docs. |
@malefice Thank yo so much! That looks like a lot of work :) LGTM, Go ahead with the docs 👍 |
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 work! jut a few minor things in the docs but overall this is 🌠
I would have preferred to use Sphinx's |
It works for now, we can revisit later. I'm going to release one last |
What does this change
Faker
can now accept multiple locales while retaining the same signature to preserve backward compatibility.What was wrong
Faker
can only handle a single locale.How this fixes it
The new
Faker
proxy class will internally create locale-specific generators as needed, and then proxy calls to the generators that can accommodate the call. Generator selection logic may also be specified via weights.Notes
Needless to say, there will be a performance penalty, and the severity of the penalty depends on:
Fixes #691, fixes #976, partial solution-ish to #453, related to #230