-
Notifications
You must be signed in to change notification settings - Fork 16.4k
i18n: add jsoncRules to handle translation key sort #51968
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
pierrejeambrun
left a comment
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.
We recently added jsonc-eslint-parser and chose to use this lib to enable parsing of json for eslint. Can you try to use it as well or maybe move everything to eslint-plugin-jsonc if not possible. (jsonc-eslint-parser was around for a longer period of time and seems more flexible, but no strong opinion here)
Also I'm surprised, it means that all our translation files are properly sorted by key, what's enforcing that ? (prettier maybe ?)
|
It looks like:
So, if an ESLint plugin needs to parse our JSON files, we should be using a JSON parser like jsonc-eslint-parser like how we use in i18nRules. Let me know if I'm mistaken, thanks!
It’s ESLint. We used the perfectionist plugin at that time but it doesn't work when files are in public folder |
pierrejeambrun
left a comment
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.
yes I think we should use jsonc-eslint-parser everywhere if possible. If not then eslint-plugin-jsonc.
agree |
92464fa to
b85c1f5
Compare
|
I've decided to keep both since:
We could consider implementing custom logic for sorting keys ourselves, which would allow us to rely solely on the parser. However, that would likely introduce additional complexity to the workflow. Let me know if you think it's worth consolidating or if I mistaken anything, but for now I feel keeping both is the more practical choice. |
135a4a6 to
757372b
Compare
pierrejeambrun
left a comment
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.
Thanks
Also after some more research it appears we need both. Thanks |
Thanks for the review and research. |
Related Issue
#51735
cc @pierrejeambrun @bbovenzi
Why
The original perfectionist can't handle json files properly in
publicfolder.How
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.