murar8 / axum_typed_multipart

Type safe multipart/form-data handling for axum.
77 stars 13 forks source link

impl Deref and DerefMut for TypedMultipart and BaseMultipart #56

Closed gengteng closed 11 months ago

gengteng commented 11 months ago

This PR implements Deref and DerefMut traits for TypedMultipart and BaseMultipart. This makes these types more ergonomic to use, allowing them to be dereferenced to the inner type.

struct Data {}
impl Data {
    pub fn modify(&mut self) {}
    pub fn read(&self) {}
}
let mut tm = TypedMultipart(Data {});
tm.modify();
tm.read();
murar8 commented 11 months ago

Hi, thank you for taking the time to contribute to the project!

From https://doc.rust-lang.org/std/ops/trait.Deref.html:

Implementing Deref for smart pointers makes accessing the data behind them convenient, which is why they implement Deref. On the other hand, the rules regarding Deref and DerefMut were designed specifically to accommodate smart pointers. Because of this, Deref should only be implemented for smart pointers to avoid confusion.

So unfortunately I will have to close this PR.

gengteng commented 11 months ago

@murar8 Dear maintainer,

Thank you for reviewing my PR and providing valuable feedback. I understand your concerns about implementing Deref and DerefMut for the extractors mainly from the semantics of Deref.

However, upon research, I've noticed that all the extractors provided by Axum itself, including Json, Query, Path, Form, as well as WithRejection and Cached from the axum-extra library, implement both Deref and DerefMut. Given the high similarity between these extractors, implementing Deref and DerefMut is likely an overall design direction of Axum.

As a 3rd party library in the Axum ecosystem, I believe following the common practices of Axum would be more user-friendly and easily accepted by users. Despite your emphasis on the semantics of Deref, for this particular library, implementing Deref and DerefMut for the extractors is primarily for convenience of use.

Therefore, I still recommend accepting this PR, as it merely follows existing Axum design patterns, as evidenced by the Deref and DerefMut implementation in the extractors provided by Axum core and axum-extra.

Thank you again for your valuable feedback! Please let me know if my understanding has any gaps and I'll respect your decision as the maintainer.

murar8 commented 11 months ago

Your point is very good, I found the issue here: https://github.com/tokio-rs/axum/issues/54 Thanks for bringing that up, I would agree that respecting the axum patterns has higher priority.

codecov[bot] commented 11 months ago

Codecov Report

Merging #56 (ce21b06) into main (2791bb0) will increase coverage by 0.11%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   96.39%   96.51%   +0.11%     
==========================================
  Files          11       11              
  Lines         361      373      +12     
==========================================
+ Hits          348      360      +12     
  Misses         13       13              
Files Coverage Δ
src/base_multipart.rs 100.00% <100.00%> (ø)
src/typed_multipart.rs 100.00% <100.00%> (ø)
gengteng commented 11 months ago

@murar8 I have modified the test function name and the commit message.