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

chore: define const of a type instead of type casting in generateLocales script #728

Closed
wants to merge 1 commit into from

Conversation

pkuczynski
Copy link
Member

@pkuczynski pkuczynski commented Mar 29, 2022

This will start catching issues with our locales definitions...
Check #729 listing the issues...

@pkuczynski pkuczynski requested a review from a team as a code owner March 29, 2022 23:04
@pkuczynski pkuczynski self-assigned this Mar 29, 2022
@pkuczynski pkuczynski added the c: bug Something isn't working label Mar 29, 2022
@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #728 (2cf4c24) into main (a37b1ef) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #728   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1924     1924           
  Lines      177022   177022           
  Branches      904      904           
=======================================
  Hits       175861   175861           
  Misses       1105     1105           
  Partials       56       56           

@ST-DDT
Copy link
Member

ST-DDT commented Mar 30, 2022

This will be in v7 at the earliest as it definitely a breaking change.

@pkuczynski
Copy link
Member Author

How come this is a breaking change?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 31, 2022

How come this is a breaking change?

If we are strict about it, we loose like 20% of our locale data categories (Guestimated).
App, buisness, cell phone, team, ...

@ST-DDT
Copy link
Member

ST-DDT commented Mar 31, 2022

And I would like to check each of them, whether they can be useful for us before deleting them.

@ejcheng ejcheng added the c: chore PR that doesn't affect the runtime behavior label Apr 11, 2022
@ST-DDT
Copy link
Member

ST-DDT commented May 3, 2022

@pkuczynski I think we should change our definitions to be extensible like our base definitions:

} & {
// Unsupported & custom modules
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[group: string]: any;
};

That way we would get rid of the as X casts, while still allowing custom entries.
Do you want to create your own PR for it or do you want me to create it?
When that is merged, we can merge this one.

@ST-DDT
Copy link
Member

ST-DDT commented May 3, 2022

Will be superseded by #915.

@pkuczynski
Copy link
Member Author

Do you want to create your own PR for it or do you want me to create it? When that is merged, we can merge this one.

Go for it. I have super limited time at the moment :(

@ST-DDT ST-DDT closed this May 4, 2022
@ST-DDT
Copy link
Member

ST-DDT commented May 4, 2022

Fixed in #915 .

@ST-DDT ST-DDT deleted the generate-locales-fix branch May 4, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working c: chore PR that doesn't affect the runtime behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants