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

disambiguate parameters (#204) #303

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

ocramz
Copy link
Collaborator

@ocramz ocramz commented Sep 24, 2023

Disambiguate request parameters (#204).

  • Adjust the Env type to have three [Param] fields instead of one
  • add captureParam, formParam, queryParam and the associated captureParams, formParams, queryParams.
  • Add deprecation notices to param and params
  • adjust examples and tests

@jfraudeau
Copy link
Contributor

Nice to see these changes coming to scotty !
If I may add a comment, I'm not sure it makes sense to call next when parseParam fails for query and form parameters. I believe it would provide better errors to just return a "bad request" in these cases.

@ocramz
Copy link
Collaborator Author

ocramz commented Sep 25, 2023

Nice to see these changes coming to scotty ! If I may add a comment, I'm not sure it makes sense to call next when parseParam fails for query and form parameters. I believe it would provide better errors to just return a "bad request" in these cases.

@jfraudeau Could you share a permalink to where is this behaviour implemented? Thank you

@jfraudeau
Copy link
Contributor

Sure : https://github.com/ocramz/scotty/blob/289ce9f9dc812edbeefaca0c773319d4d23a7675/Web/Scotty/Action.hs#L276

@ocramz
Copy link
Collaborator Author

ocramz commented Sep 25, 2023

@jfraudeau Good catch! But applying your suggestion would be a user-visible change, even though I don't know whether anybody actually relied on the current (buggy) behaviour or they simply worked around it.

@jfraudeau
Copy link
Contributor

@ocramz I think it should be ok if the change only applies to the new formParams and queryParams

@ocramz
Copy link
Collaborator Author

ocramz commented Sep 25, 2023

I took a stab at it and added a few tests, @jfraudeau please take another look when you have time. Thanks!

Web/Scotty/Action.hs Outdated Show resolved Hide resolved
@ocramz ocramz merged commit 36fda87 into scotty-web:master Sep 25, 2023
@ocramz ocramz deleted the disambiguate-params-#204 branch September 25, 2023 19:38
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