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

refactor: split up huma.Register #705

Merged
merged 16 commits into from
Jan 16, 2025

Conversation

b-kamphorst
Copy link
Contributor

The current huma.Register function is 900+ lines of code and has up to 9 levels of nesting. While working on #694 I found it really hard to understand the logic. In my effort to understand the function, I performed several refactorings and I thought that you may appreciate them as well.

Surely some parts are a matter of taste -- I don't mind changing them.

Looking forward to your thoughts.

@b-kamphorst
Copy link
Contributor Author

The diff is quite large, so I made an effort to clean up the branch history. I think it may be easiest to review this PR by traversing the commits. Every commit passes the tests (git rebase main -x 'go test ./...'), as should be expected in a refactoring.

Before I cleaned up the history I made a change, passed the tests, and later figured out that it broke the flow of logic for the TextUnmarshaler. I added a test to prevent that from happening again. This just reminds us that even refactor efforts that pass all tests should be reviewed with care :)

@danielgtaylor
Copy link
Owner

@b-kamphorst this has been on my ToDo list for ages, so 1. Thank you so much for taking the time to do this and 2. Sorry I didn't get to it earlier!

Looks like there is a small conflict I may be able to resolve, but I'll need to take some time and review. Originally I mashed much of this code together in an effort to keep it performant, so I will run some benchmarks too. Give me a couple days please - I do want to merge this cleanup!

@b-kamphorst
Copy link
Contributor Author

No worries, I appreciate your devotion to this wonderful project and I am glad that this may be of value. I'll rebase on main to resolve the minor conflict tomorrow.

Please take your time. Looking forward to your feedback.

@b-kamphorst b-kamphorst force-pushed the refactor-huma-register branch from 045f37e to e09d4a2 Compare January 13, 2025 20:28
@b-kamphorst b-kamphorst force-pushed the refactor-huma-register branch from e09d4a2 to 1ce45cb Compare January 13, 2025 20:46
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 94.17476% with 42 lines in your changes missing coverage. Please review.

Project coverage is 92.90%. Comparing base (fe78329) to head (1ce45cb).

Files with missing lines Patch % Lines
huma.go 94.00% 31 Missing and 9 partials ⚠️
formdata.go 96.29% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
+ Coverage   92.78%   92.90%   +0.12%     
==========================================
  Files          22       22              
  Lines        4917     4964      +47     
==========================================
+ Hits         4562     4612      +50     
+ Misses        308      305       -3     
  Partials       47       47              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielgtaylor danielgtaylor requested a review from Copilot January 16, 2025 15:59
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

formdata.go:180

  • The error message 'Failed to open file' is not very descriptive. Consider providing more details to help with debugging.
return FormFile{}, &ErrorDetail{Message: "Failed to open file", Location: location}

formdata.go:94

  • The new behavior introduced in the Decode function should be covered by tests.
func (m *MultipartFormFiles[T]) Decode(opMediaType *MediaType) []error {

huma_test.go Show resolved Hide resolved
@danielgtaylor
Copy link
Owner

@b-kamphorst thanks for all this work! I've reviewed the code and it looks good. The one issue I found is that benchmarks showed an increase in dynamic memory allocations per request and I was able to git bisect it to 78b0cc4, specifically the change in splitting query param strings before they might be needed. I've pushed a fix to move that logic to only be applied if the underlying parameter is actually an array of values. The performance is now equal to before the PR, so I am going to merge. Thanks again 🥇

@danielgtaylor danielgtaylor merged commit 1c3924e into danielgtaylor:main Jan 16, 2025
3 checks passed
@b-kamphorst
Copy link
Contributor Author

That's nice! If you have any recommended sources on (that kind of) Go benchmarking then I would be interested. I'm a couple of months into the language.

Glad to hear you valued the refactoring, happy to help!

@b-kamphorst b-kamphorst deleted the refactor-huma-register branch January 21, 2025 13:51
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