-
Notifications
You must be signed in to change notification settings - Fork 1
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
Encapsulation / separate of concern #4
Comments
It's a lot to agree to up-front. I'd suggest breaking it down into multiple PR's. I definitely think having stream factories is over-engineering. |
please also see https://github.com/Lewiscowles1986/rfc1867/blob/521db9ff01d8c19e4a43bef69068d8bf33a8d9ca/README.md for stated goals. |
I agree, and i would have posted multiple issues if i had any ideas on how to split it. I'll try multiple progressives PR
You already use guzzle stream factory :3 It planned to open another issue and pr on this subject (as it is indeed a separate issue) I've seen the goals, and stream factories are the only solution i see to decouple from guzzle. |
Wow, "epic" tag :D |
Btw I've been through the code now.
I don't think as a library this is poorly designed or arduous for someone to get their head around.
Yes a class that only pulls in other things and cannot be extended already has a coupling to an externally maintained perfectly good, tested stream factory, which complies with PSR-7 StreamInterface (works were done this AM to bind to StreamInterface instead of Stream). I'm still open to seeing what you come up with, but try to keep it simple, lightweight. |
Don't get me wrong, i've never said that : the library is easy to understand and use, and will work perfectly in most of use cases. My only point was to present a different class diagram to better separate class responsibilities (while trying to keep it simple to use), decouple it from guzzle, and remove implicit non-injected dependencies And, obviously, that is only an idea, only my opinion, and will be only few pr proposals, and in the end, only your choice to merge or not. I don't plan to start a whole community drama to justify a fork project :D |
Here's a first draft, where I kept as much original code as I could, focusing on providing a class skeletton. I encountered few problem writing this :
That basically means that I may not be able to provide a second small PR to show my ideas, as I'll propably have to rewrite the nesting/disposition logic, the boundary generation AND the encoder chain in the same run. But if you think I'm heading into an interresting direction I should be able to provide this tomorrow. |
Not so big finally. |
In the current implementation, the nodes classes have multiple responsibilities :
So here's a possible séparation :
While having encoders returning streams may seem counter intuitive, that would allows to send files as non buffered streams, saving memory, and the formdata encoder could even compose a chunked stream with the streams returned by sub encoders
The text was updated successfully, but these errors were encountered: