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

use const instead of enum #535

Merged
merged 8 commits into from
Apr 27, 2023
Merged

use const instead of enum #535

merged 8 commits into from
Apr 27, 2023

Conversation

lublak
Copy link
Contributor

@lublak lublak commented Apr 11, 2023

Please:

  • Make your pull request atomic, fixing one issue at a time unless there are many relevant issues that cannot be decoupled.
  • Provide a test case & update the documentation in the Readme.md

fixes #534

@lublak lublak changed the title Patch 1 use const instead of enum Apr 11, 2023
@domoritz
Copy link
Collaborator

Please remove the npm lock file

@lublak
Copy link
Contributor Author

lublak commented Apr 13, 2023

@domoritz done

@domoritz
Copy link
Collaborator

Tests are failing

@lublak
Copy link
Contributor Author

lublak commented Apr 16, 2023

@domoritz can you try again

@domoritz
Copy link
Collaborator

Why should we make this configurable? I feel it's a better default and would like it always enabled. What do you think?

@lublak
Copy link
Contributor Author

lublak commented Apr 17, 2023

@domoritz then it is not compatible with your tests. (backwards compatible?)

@domoritz
Copy link
Collaborator

Of we think this is better then we can release a new breaking version.

One question? How does this work if you have an enum with one value?

@domoritz
Copy link
Collaborator

And speaking of tests, we need one.

@lublak
Copy link
Contributor Author

lublak commented Apr 17, 2023

@domoritz the issue ist currenlty: The complete project doesn't work. If i just try npm test, the tests are currently failing (and no i have not really time to fix all issues).
Okay a breaking version can be fine for this.
If you have a enum with one value it works fine with most stuff.
But const is here a better solution. After all, that's what const is for.

@domoritz
Copy link
Collaborator

You can just add one test and run that wither by marking the test as .only or commenting out the others.

@lublak
Copy link
Contributor Author

lublak commented Apr 18, 2023

@domoritz i changed now the tests and its now a breaking change.

@domoritz
Copy link
Collaborator

Please fix the tests so we can merge this.

@lublak
Copy link
Contributor Author

lublak commented Apr 27, 2023

@domoritz yes!
The issue is, if i run my test local i can not see all issues directly. So i only can see if its fails here.
My local test looks like this (in the workflow the tests works fine, currently also fails but with correct error message):

grafik
grafik

but i hope i can fix it without testing it.

@lublak
Copy link
Contributor Author

lublak commented Apr 27, 2023

@domoritz can you try again? :)

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.

enums with single values should be "const"
2 participants