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

List of features for the next big releases #620

Closed
5 of 14 tasks
iMacTia opened this issue Oct 18, 2016 · 7 comments
Closed
5 of 14 tasks

List of features for the next big releases #620

iMacTia opened this issue Oct 18, 2016 · 7 comments

Comments

@iMacTia
Copy link
Member

iMacTia commented Oct 18, 2016

Faraday 1.0

@grosser
Copy link
Contributor

grosser commented Mar 26, 2017

I'm looking for streaming support ... is anyone working on 1.0 ? ... if not why was the other PR closed 😕

@iMacTia
Copy link
Member Author

iMacTia commented Mar 27, 2017

Hi @grosser, reasons are expressed in this #604 (comment).
Main reason was that the PR was only compatible with Net::HTTP adapter and the fact that streaming have been marked for v1.0.
There's no roadmap for v1.0 at the moment, so no one actively working on streaming for the time being.

@jcoyne
Copy link

jcoyne commented Jul 11, 2017

@iMacTia I'm a bit disappointed that you say "no one actively working on streaming" because it seems you squashed work on #604 (also #522, #461) by saying "Hold tight, this will be in the next release", but then not working on it. Why not accept the communities changes considering there doesn't seem to be momentum by the core team?

@iMacTia
Copy link
Member Author

iMacTia commented Jul 13, 2017

@jcoyne I didn't mean that streaming is not in yet BECAUSE "no one actively working on streaming". I'm fully aware that the fact no one is working on it is mainly my fault. However, I've explained why I pushed back on #604 and the problem was not the implementation.
In order to get 604 merged in one of the following needs to happen first:

  1. Support for all the other adapters is added (right now only Net::HTTP is supported)
  2. We wait for version 1.0 because we might remove direct support for other adapters and gemify them into external projects. That would make Streaming requests for Net::HTTP #604 mergeable as it is now, but unfortunately it has not been agreed internally yet.

I apologise for being slow and not having enough time to invest in working on any of the above solutions.
I understand you'd be happy with having only #604 merged because you probably only need support for Net::HTTP, but that's not how it works when you have to maintain a gem and I can't just make it that simple.

@jcoyne
Copy link

jcoyne commented Jul 13, 2017

@iMacTia I don't expect that you throw all your effort into this release, I'm just frustrated about the chilling effects of closing/denying pull requests that several people have worked on and have no known technical problems. If you don't have the time, doesn't it make sense to let others help?

I understand why support for all other adapters would be desirable, but I think you are overly strict with your interpretation the adapter pattern. The adapter pattern demands consistency of the manner you do any one interaction, but I wouldn't say it demands that every adapter support every feature. There are many useful libraries out there that use this looser definition (e.g. edgeapi.rubyonrails.org/classes/ActiveJob/QueueAdapters.html#module-ActiveJob::QueueAdapters-label-Backends+Features)

@iMacTia
Copy link
Member Author

iMacTia commented Jul 13, 2017

To be honest, I personally agree on your last sentences.
I like the way ActiveJob works and the fact that you can add compatible gems to your app and just use them as a QueueBackend (e.g. Sidekiq).

On the other side, this is not the current situation for Faraday, it's not the original vision of the core team and it's not what everyone have been used to.
Just to make an example to you, in #486 a user is complaining about exactly this issue:

I'd expect Faraday to just work when swapping out adapters.

And that's what @mislav and any other core team member enforced from the beginning.
So I hope you'll understand, as I totally understand your needs, that as the last member joining Faraday I can't simply throw the past decisions in the bin and start doing what I want. Which means that streaming have to be supported by all adapters before I can merge it in the 0.x flow.
Examples of other PRs closed for the same reason: #485, #498, #339 (comment)

Faraday 1.0 is different, that gives me much more freedom (I do have to preserve backward-compatibility as much as possible though, it doesn't mean I can do whatever I want 😅). And that's why I suggested to drop native support for all adapters, but rather moving them into external gems like what happened with Thypoeus. This will have a lot of advantages and will make the structure much more similar to ActiveJob, justifying things like partial-support.
For this reason, I see streaming as a 1.0 feature and I had to freeze works on it for the time being.
It will take much longer, but that means doing things properly to me. I would like to make it as clear as possible: I'm not closing PR for the sake of wasting anyone's time, I'm simply trying to help pushing this project forward (we would still be 0.9.x otherwise) honouring the core team vision.

If you don't have the time, doesn't it make sense to let others help?

Just a final note on this: EVERYONE is welcome to help. That's how Open Source works! However, we should also respect the core team's vision when we contribute to something. We can agree with them as well as disagree (I disagree with them on some points too!), but we have to respect their choices because if Faraday is what it is today, it's surely thanks to the many contributors, but also thanks to the core team managing all the Issues/PRs and progressing the project in a clear and logical way (and believe me, the latter is much more time consuming than sporadic contributions). If it wasn't for them filtering or adjusting contributors inputs, we might know a completely different Faraday today, and I'm not sure it would be better than the one we actually know.

@iMacTia
Copy link
Member Author

iMacTia commented Feb 7, 2018

This issue has now been converted into a project.

@iMacTia iMacTia closed this as completed Feb 7, 2018
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

No branches or pull requests

3 participants