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

fix(types): locale proxy #2099

Merged
merged 5 commits into from
Apr 28, 2023
Merged

fix(types): locale proxy #2099

merged 5 commits into from
Apr 28, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Apr 26, 2023

The main sources don't show the error, but the test code does:

grafik
(e.g. date.spec.ts)

@ST-DDT ST-DDT added c: bug Something isn't working p: 2-high Fix main branch s: needs decision Needs team/maintainer decision labels Apr 26, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Apr 26, 2023
@ST-DDT ST-DDT requested review from a team April 26, 2023 22:11
@ST-DDT ST-DDT self-assigned this Apr 26, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 26, 2023

I'm not sure whether this is the correct solution.

@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision and removed s: needs decision Needs team/maintainer decision labels Apr 26, 2023
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #2099 (31aede5) into next (8395e69) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2099      +/-   ##
==========================================
- Coverage   99.59%   99.59%   -0.01%     
==========================================
  Files        2567     2567              
  Lines      243368   243371       +3     
  Branches     1253     1247       -6     
==========================================
- Hits       242394   242380      -14     
- Misses        947      964      +17     
  Partials       27       27              
Impacted Files Coverage Δ
src/locale-proxy.ts 100.00% <100.00%> (ø)
src/modules/helpers/index.ts 99.07% <100.00%> (-0.01%) ⬇️
src/modules/word/filterWordListByLength.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Apr 27, 2023

The main sources don't show the error, but the test code does [...]

I'm pretty sure that is because the test files are not concluded in the tsconfig.json. I don't actually know which config files they use, but they seem to have different TS config settings. If you include 'test/**/*' in the tsconfig.json file the error is gone.

If you then activate the strictNullChecks rule, the error is there again and additionally in the source files. So we essentially missing strict configuration settings.

@Shinigami92
Copy link
Member

I checked if we could use any instead of unknown, but then we are affected by not being refactor safe 😕
So if someone really uses the for custom definitions, they are forced to cast 🤷

@Shinigami92 Shinigami92 removed the s: needs decision Needs team/maintainer decision label Apr 27, 2023
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to start adding TS compile tests to the project. Just an idea.

@Shinigami92
Copy link
Member

We may want to start adding TS compile tests to the project. Just an idea.

No, we just need to start using strict
see my already started efforts at

we dont need tests, we need more safe checked TS itself

@ST-DDT ST-DDT enabled auto-merge (squash) April 28, 2023 13:10
@ST-DDT ST-DDT merged commit 971f363 into next Apr 28, 2023
@Shinigami92 Shinigami92 deleted the fix/types/locale-proxy branch April 28, 2023 13:18
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 p: 2-high Fix main branch
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants