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

Improve enumToMap typings #278

Merged
merged 5 commits into from
Jan 30, 2024
Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 29, 2024

This improves the typings and also removes one typing issue reported by biome.

the only real runtime change is replacing the regex with a direct property access check.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 30, 2024

remaining linting issues will be fixed in #277

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 30, 2024

@ShogunPanda
It seems that node test runner has some issue under windows (from the mocha removal PR). Working on it to get it fixed.

@ShogunPanda
Copy link
Contributor

Thanks!

About this PR, I think we should instead get rid of enums completely (which I find very evil in TypeScript), rather than try to fix a mapping function.
I will work on this in like 30 min. WDYT?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 30, 2024

I personally dislike named enums in typescript too, as the transpiled javascript result is very much unreadable and imho garbage. But I was hesitating, because theoretically replacing enums can be done easily by using also interfaces and basically duplicating the structure, and did not know if you would actually accept it or if it is how you would solve it.

But If you have a better approach, than I have no objection ;).

I just think that we could get the CI green if you merge this PR. Then we have a clean slate and you can make your change on top of it.

I will merge now main into this PR to see the green pipeline :D

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 30, 2024

Its green. Yay. :D

@ShogunPanda ShogunPanda merged commit e486f51 into nodejs:main Jan 30, 2024
11 checks passed
@Uzlopak Uzlopak deleted the improve-enumToMap branch January 30, 2024 09:28
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.

2 participants