-
Notifications
You must be signed in to change notification settings - Fork 157
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
Remove public methods from IdNumber
that are not intended to be directly called
#1214
Comments
If that's the intent then I'm in agreement. I haven't been working on the project long enough to know. |
Makes sense. I don't think we have a super clear agreement about what is our public api vs our more internal. What would you propose to limit the visibility of these methods? Make them package protected? Or something else? |
I think we don't need these methods at all.
I can submit a minimal pull request to demonstrate this approach. |
yes that probably makes sense why it was done in current way - probably nobody knows since it was done in javafaker first and those contributors are not active in both communities...
yes,go ahead The only thing to note since they are public for a long time then first need to mark as deprecated and then remove later |
…try code Thus, we can add ID numbers for new countries just by implementing "IdNumbers" without need to add public methods like "validZhCNSsn()", "validEstonianPersonalCode()", "validBulgarianPersonalCode()" to class "IdNumber". Now all country-specific ID number generators are loaded from file "src/main/resources/META-INF/services/net.datafaker.idnumbers.IdNumbers" using a standard Java mechanism "service loader". + rename existing "*IdNumber" classes to human-readable, e.g. "ZhCnIdNumber" to "ChineseIdNumber".
there is no point in having method `option(String... options)` when we already have generic method `option(E... options)`.
... instead of introducing a new method.
…try code Thus, we can add ID numbers for new countries just by implementing "IdNumbers" without need to add public methods like "validZhCNSsn()", "validEstonianPersonalCode()", "validBulgarianPersonalCode()" to class "IdNumber". Now all country-specific ID number generators are loaded from file "src/main/resources/META-INF/services/net.datafaker.idnumbers.IdNumbers" using a standard Java mechanism "service loader". + rename existing "*IdNumber" classes to human-readable, e.g. "ZhCnIdNumber" to "ChineseIdNumber".
... instead of introducing a new method.
@asolntsev may be for future: I would suggest squashing commits while merging since it will simplify navigation through commits history |
@snuyanzin Thanks. Yes, in this case I should squash all commits. |
yep, in generic case it depends on the context |
For my two cents it's the overall contribution that matters to the project not the individual bits of work/correction. So I'm in favour of squash and merge or using amending commits. |
Describe the bug
It's rather a design issue, not a bug.
I don't like the fact that class
net.datafaker.providers.base.IdNumber
has all these public methods:validSvSeSsn()
validEnZaSsn()
inValidEnZaSsn()
invalidSvSeSsn()
validEsMXSsn()
invalidEsMXSsn()
validEstonianPersonalCode()
invalidEstonianPersonalCode()
validAlbanianPersonalCode()
invalidAlbanianPersonalCode()
validMoldovaPersonalCode()
invalidMoldovaPersonalCode()
validBulgarianPersonalCode()
invalidBulgarianPersonalCode()
Problem 1:
I assume they are NOT intended to be explicitly called from client's code.
Instead, clients should generate corresponding ID numbers using locale/country:
Problem 2:
Having these methods as public makes it impossible to extend functionality of DataFaker. Like, when user adds a custom JAR to classpath that implements yet another language/country.
Expected behavior
It should be possible to add "yet another country" without adding any new public methods.
It should be enough just to add file "LANG.yml" and probably some (package-private) classes like
BulgarianIdNumber
.Versions:
@bodiam @snuyanzin thoughts?
The text was updated successfully, but these errors were encountered: