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.

Sign up for GitHub

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

Jump to bottom

Path param int deserialization #381

Merged
merged 11 commits into from
Feb 3, 2025
Merged

Path param int deserialization #381

merged 11 commits into from
Feb 3, 2025

Conversation

thezbm
Copy link
Contributor

@thezbm thezbm commented Feb 2, 2025

closes #377

@thezbm thezbm marked this pull request as ready for review February 2, 2025 10:11
@thezbm
Copy link
Contributor Author

thezbm commented Feb 2, 2025

@EwenQuim Am I on the right track? I basically mimicked the implementation of PathParamIntErr and PathParamInt. What else should I do?

EwenQuim
EwenQuim requested changes Feb 2, 2025
View reviewed changes
Copy link
Member

@EwenQuim EwenQuim left a comment

Choose a reason for hiding this comment

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

I think that's good! Thank you very much 🎉

Maybe try to place the logic in a single place?

And most of all, please add test to prove it works and ensure it never gets broken in future versions :)

@thezbm
Copy link
Contributor Author

thezbm commented Feb 2, 2025

Tests are added.

thezbm
thezbm commented Feb 2, 2025
View reviewed changes
ctx.go Show resolved Hide resolved
dylanhitt
dylanhitt reviewed Feb 2, 2025
View reviewed changes
ctx.go Outdated Show resolved Hide resolved
ctx.go Show resolved Hide resolved
ctx.go Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
EwenQuim
EwenQuim approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@EwenQuim EwenQuim left a comment

Choose a reason for hiding this comment

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

I think we'll merge this PR and then

  • add tests for PathParamIntErr
  • remove PathParamIntErr and make PathParamInt return zero-value or an error

ctx_test.go Show resolved Hide resolved
@EwenQuim
Copy link
Member

EwenQuim commented Feb 3, 2025

Also, please run make as described in the Contributing guide

@EwenQuim EwenQuim added this to the v0.18 milestone Feb 3, 2025
@EwenQuim EwenQuim merged commit 0ba4092 into go-fuego:main Feb 3, 2025
5 checks passed
@thezbm
Copy link
Contributor Author

thezbm commented Feb 3, 2025

Thank you. I'll be careful next time.

@EwenQuim
Copy link
Member

EwenQuim commented Feb 3, 2025

Thank you @thezbm for this PR!

High quality work here :) 🎉

This was referenced Feb 5, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylanhitt dylanhitt dylanhitt left review comments

@EwenQuim EwenQuim EwenQuim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.18
Development

Successfully merging this pull request may close these issues.

Deserialize path param to int
3 participants
@thezbm @EwenQuim @dylanhitt