search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

refactor: only expose getters #1407

Closed Banyc closed 2 years ago

djc commented 2 years ago

Not sure how @Ralith feels, but personally this seems to me like using 15 more lines of code on a firming up an internal abstraction, which we don't really need since we're already pretty decent at protecting our internal abstractions.

I don't think it's a net improvement.

Banyc commented 2 years ago

My proposal is to further separate components inside quinn-proto. The additional 15 lines is the cost of encapsulating Recv and protect its fields from invalid direct modification in the future.

It is based on the premise that client code to Recv does not understand the invariant of Recv. If a field is pub, then all possible direct modifications on this field should always agree with the invariant of Recv. If there exists one direct modifications on this field that violates the invariant of Recv, the field should not be pub.

The fewer invariants we have to consider, the fewer mental burdens we have and more energy to consider the business logic. Also there will be fewer bugs.

djc commented 2 years ago

I understand your proposal. I feel that in this case the benefit you propose, while real, is small and not worth the cost: IMO the mental burden by the current setup is negligible. I don't think we've found any bugs so far due to having porous abstraction boundaries, so I'm skeptical of your claim that there will be fewer bugs.

Banyc commented 2 years ago

This commit is a brief example on protecting fields, so the reduction of concerning invariant is negligible. But I want this pattern to be used on all code in quinn-proto, not only on Recv. It will make a huge difference.

I'm convinced by the debunk of the "fewer bugs" claim.

djc commented 2 years ago

But I want this pattern to be used on all code in quinn-proto, not only on Recv. It will make a huge difference.

I'm sorry, why should I care what you want? I'm unconvinced by your claim that it will make a huge difference.

Banyc commented 2 years ago

@djc No offense here. I just made a proposal. No sarcasm at all. I'm not a native English speaker. I'm sorry for the misunderstanding during the discussion. I'm not intended to make anyone unhappy.

I'm convinced by the debunk of the "fewer bugs" claim.

With this, I just want to convey the fact that I believe my claim is wrong and I will not focus on this claim anymore, just it. The further communication will not based on this claim. It indicates that the first paragraph is not related to the "fewer bugs" claim. So the "difference" here is not about "fewer bugs", but just for the "mental burdens" part.

Banyc commented 2 years ago

You are not wrong by unconvincing "it will make a huge difference" so I now should close this merge request because you are the owner of the repo.