murar8 / axum_typed_multipart

Type safe multipart/form-data handling for axum.
84 stars 14 forks source link

fix: dont panic inside implementation #51

Closed HigherOrderLogic closed 1 year ago

HigherOrderLogic commented 1 year ago

As the PR title said. The reason for this is that I don't want my code to panic because of some macro implementation.

Todo

codecov[bot] commented 1 year ago

Codecov Report

Merging #51 (5349a26) into main (b6493ca) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #51   +/-   ##
=======================================
  Coverage   96.31%   96.31%           
=======================================
  Files          11       11           
  Lines         353      353           
=======================================
  Hits          340      340           
  Misses         13       13           
Files Changed Coverage Δ
macros/src/impls/try_from_multipart.rs 93.33% <100.00%> (ø)
src/typed_multipart_error.rs 100.00% <100.00%> (ø)
murar8 commented 1 year ago

Hi, sorry for the delay, thank you for your contribution! Unfortunately I was not aware that multipart/form-data supports nameless fields. Your implementation makes perfect sense to me because I really don't see a way to assign a field to a struct field without having a name. I would add a couple tests though so we don't have regressions. If you don't have time I can add them during the weekend.

HigherOrderLogic commented 1 year ago

What do you have in mind for a better error?

I'm thinking about providing some extra information with the error, like the data that go with that nameless field, but currently I'm struggling to do that.

murar8 commented 1 year ago

I'm thinking about providing some extra information with the error, like the data that go with that nameless field, but currently I'm struggling to do that.

Do you think that would be useful? I can't think of a use case for the data, in my opinion the way you wrote it is good.

murar8 commented 1 year ago

Since I have some free time I will move the development of this fix to #53, thank you for the contribution!