Skip to content

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

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

krsriq
Copy link
Contributor

@krsriq krsriq commented Dec 4, 2020

Adds missing documentation for a couple of locales.

Signed-off-by: Daniel Schmelz <daniel@schmelz.org>
Copy link
Member

@bram-pkg bram-pkg left a 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!

### `Faker\Provider\en_IN\Address`

```php
echo $faker->locality; // "Vaishali Nagar"
Copy link
Member

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.)

Copy link
Contributor Author

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)?

Copy link
Contributor

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?

Copy link
Member

@bram-pkg bram-pkg Dec 4, 2020

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@krsriq krsriq Dec 5, 2020

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>
@pimjansen pimjansen merged commit cb02426 into FakerPHP:main Dec 7, 2020
@krsriq krsriq deleted the add_missing_docs2 branch December 11, 2020 12:06
krsriq added a commit to krsriq/fakerphp.github.io that referenced this pull request Dec 19, 2020
* add missing documentation

Signed-off-by: Daniel Schmelz <daniel@schmelz.org>

* add missing documentation

Signed-off-by: Daniel Schmelz <daniel@schmelz.org>
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.

3 participants