-
Notifications
You must be signed in to change notification settings - Fork 2
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
Tests for "fix(parameters): Support int numerals #38" #30
Conversation
nickvergessen
commented
Nov 10, 2023
•
edited
Loading
edited
- Fix extracted to fix(parameters): Support int numerals #38 - This PR is kept to merge the tests ones the base is there
- Based on feat(tests): Add a demo app for testing #59 for testing
Before | After |
---|---|
… target clients Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
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.
I wonder if it would be possible to merge this with the string enum handling to reduce duplicate code since they are both quite similar.
I don't have a usecase atm, but i wonder what happens if you mix int+string on one parameter, will try after lunch |
I think it should resort to using |
You mean? |
Yes, although now I'm not sure how it will handle it 😅 |
Let's move that to a follow up, as mixed types seem to not work at all:
Generates:
|
@nickvergessen can you set the target branch to the main branch and drop the scope commits so we can get this merged already? |
Well doesn't work with the test files. I can make a new PR against master with only the source code, but we should keep a second PR/branch/commit to merge the tests as soon as that is possible. |
Fine by me |
0358899
to
93a6490
Compare
ac83995
to
94962d4
Compare
Test added in #59 |