-
Notifications
You must be signed in to change notification settings - Fork 99
Add more missing locale documentation #24
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
Conversation
Signed-off-by: Daniel Schmelz <daniel@schmelz.org>
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.
Left you a comment, looks good to me otherwise!
docs/locales/en_IN.md
Outdated
### `Faker\Provider\en_IN\Address` | ||
|
||
```php | ||
echo $faker->locality; // "Vaishali Nagar" |
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.
Can you add the rest of the methods here too? (state
, etc.)
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.
state
I left out here on purpose to keep it consistent - many Address
providers have a state
method, but only de_AT a documentation for that - I've added it now here.
Should I add the state
to every Address
doc (in a separate PR)?
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.
bramceulemans aren't we documenting too much that way? Since state is part of the address core it's a bit overkill to document it on every locale.
Though it is not wrong and even could be useful since it gives a more representative output, it still feels a lot to maintain.
Views?
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.
I did not notice that state was in the core Address provider, if that's the case, ignore my comment.
Only additional methods outside the core should be documented in the locale-specific 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.
Ah lol nvm it is not strange enough.
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.
Ah lol nvm it is not strange enough.
Yes - so we could either leave it in the docs here as is done in this PR for en_IN (and add it to the other localized Address
documentation for consistency), or add a state
method to the core?
I counted 23 (of 47) localized Address providers that have a state method. There are probably more that have a method that's comparable to state
, but differently named.
Signed-off-by: Daniel Schmelz <daniel@schmelz.org>
* add missing documentation Signed-off-by: Daniel Schmelz <daniel@schmelz.org> * add missing documentation Signed-off-by: Daniel Schmelz <daniel@schmelz.org>
Adds missing documentation for a couple of locales.