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

feat(dynamite): always send parameters even when being the default #1292

Merged
merged 3 commits into from
Dec 17, 2023

Conversation

Leptopoda
Copy link
Member

fixes: #1281

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

The change to the parameter doesn't make sense. Only the check if the value matches the default value has to be removed.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Looking at the fixtures it seems like empty values are added?

Also the fixtures need to keep the regexes otherwise they don't work.

@Leptopoda
Copy link
Member Author

Looking at the fixtures it seems like empty values are added?

Also the fixtures need to keep the regexes otherwise they don't work.

I'll double check. Maybe I need to do everything at once in a big PR. UriTemplate does not do so so probably some issue got introduced when rebasing.

@Leptopoda
Copy link
Member Author

Looking at the fixtures it seems like empty values are added?

Also the fixtures need to keep the regexes otherwise they don't work.

Can you show me where? I can't find any broken one.

@Leptopoda
Copy link
Member Author

I think you where referring to the parameters that default to an empty string.

I'll fix the fixtures (re add the reggexes that got removed for the parameters) and try to figure out why the tests are failing.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

LGTM, but it would have been easier if you explained from the start why the default values can no longer be present in the method header. Just from this diff alone it doesn't make sense so it is better to explain why you did it.

.vscode/settings.json Outdated Show resolved Hide resolved
@Leptopoda Leptopoda force-pushed the feat/dynamite/always-send-default-parameters branch from c05a18a to 181ebae Compare December 15, 2023 14:20
@Leptopoda Leptopoda enabled auto-merge December 15, 2023 14:22
@provokateurin
Copy link
Member

I find it still confusing that there is no equals sign when the value is an empty string but if that is correct I don't care 🤷‍♀️

@Leptopoda Leptopoda disabled auto-merge December 15, 2023 14:33
@Leptopoda
Copy link
Member Author

I'll take a look. Totally forgot to verify that.
Thanks for mentioning

@Leptopoda
Copy link
Member Author

Ok it is wrong
https://datatracker.ietf.org/doc/html/rfc6570#section-3.2.9

   {&x,y,empty}       &x=1024&y=768&empty=

it will get even funnier as we must also add the =.
I don't think this is possible in the current way. Should I close this PR and just have both commits in the other one?

@provokateurin
Copy link
Member

Yes I think we need to have both changes at once. I think you could change the target branch of the other PR to this branch and then we merge the other PR into this one and merge this PR in the end

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Please rebase on the main branch to ensure everything is working correctly.

@Leptopoda Leptopoda force-pushed the feat/dynamite/always-send-default-parameters branch from 334d403 to 06e2eef Compare December 16, 2023 17:09
@Leptopoda Leptopoda enabled auto-merge December 16, 2023 17:10
@Leptopoda
Copy link
Member Author

Leptopoda commented Dec 16, 2023

This is odd 🤔
on 27.0 we now get 0 Items back for the dashboard. Maybe an upstram bug @provokateurin?

@provokateurin
Copy link
Member

This is really strange indeed. I can reproduce it locally 🤔

@provokateurin
Copy link
Member

Also fails on the main branch. Switching to 27.0.1 works

@provokateurin
Copy link
Member

I rebuild the image without any cache locally and it works again...

@provokateurin
Copy link
Member

I deleted the tagged version of the image and restarted the CI and now it works 🤷‍♀️

@Leptopoda Leptopoda merged commit 3b60a60 into main Dec 17, 2023
8 checks passed
@Leptopoda Leptopoda deleted the feat/dynamite/always-send-default-parameters branch December 17, 2023 09:56
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.

send default parameters when explicitly specified.
2 participants