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

add "locale" as a possible semantic pull request type #3179

Closed
matthewmayer opened this issue Oct 13, 2024 · 9 comments
Closed

add "locale" as a possible semantic pull request type #3179

matthewmayer opened this issue Oct 13, 2024 · 9 comments
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent
Milestone

Comments

@matthewmayer
Copy link
Contributor

It is a locale PR. So I would love a locale(module): nomenclature. Until then it is I'm fine with either of those.

Originally posted by @ST-DDT in #3173 (comment)

@matthewmayer
Copy link
Contributor Author

This would be used when just adding or removing locale data in a pull request
e.g. #3173 #3154 #3142

Would this simply be a matter of adding locale in

https://github.com/faker-js/faker/blob/next/.github/workflows/semantic-pull-request.yml

?

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: infra Changes to our infrastructure or project setup s: needs decision Needs team/maintainer decision labels Oct 13, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 13, 2024
@Shinigami92
Copy link
Member

Would this simply be a matter of adding locale in

No it wont, and it would even conflict with the current infra setup of producing separate sections for Changelog

e.g. https://github.com/faker-js/faker/blob/next/CHANGELOG.md#new-locales

@ST-DDT
Copy link
Member

ST-DDT commented Oct 14, 2024

@Shinigami92
Copy link
Member

Could we first discuss why the one is better than the other?

Right now we are sure what a fix and what a feat is
This has effect not only of incrementing the minor or biggie number, but also separate between updated and new locales

The module or locale (e.g. de) is also written into the commit message

Updating this process required also a search through the current contribution guide, so we do not forget something to update

My personal though is currently to not touch this process, but I'm by far not the only maintainer and maybe there is a benefit that I currently don't see

@ST-DDT
Copy link
Member

ST-DDT commented Oct 14, 2024

My point of contention is, that we use the locale label, but the feat/refactor prefix.
Only for fixes we use both.

@matthewmayer
Copy link
Contributor Author

I'm not too bothered whether we add a new label or not, maybe it's just a question of updating the documentation to make it clearer which one to pick?

@Shinigami92
Copy link
Member

Ah now I see the refactor(vehicle): add more non-US vehicle manufacturers
because only src/locales/en/vehicle/manufacturer.ts were effectively touched for the outside-consumer, it should have been a refactor(locale): add more non-US vehicle manufacturers
and as you can see, the vehicle was already in the commit message and don't need to be addressed in commit scope a second time

But it is merged already 🤷, so better luck next time

@matthewmayer
Copy link
Contributor Author

Refactors which add new data to existing locales seem different to refactors which actually change code (but don't change the output). But maybe it's fine to call them both refactor.

@ST-DDT
Copy link
Member

ST-DDT commented Oct 26, 2024

Team Decision

  • We will keep it as is for v9.x

@ST-DDT ST-DDT closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2024
@xDivisionByZerox xDivisionByZerox removed the s: needs decision Needs team/maintainer decision label Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent
Projects
None yet
Development

No branches or pull requests

4 participants