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

Conditionally require faraday/multipart #393

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ajGingrich
Copy link

Only Require Faraday Multipart if Faraday Version is greater than 2 because Faraday V1 includes the gem already.

#392

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?

@ajGingrich ajGingrich force-pushed the only-require-faraday-multipart-for-faraday-2 branch from 96c38fe to 85a63ef Compare December 1, 2023 18:07
Only Require Faraday Multipart if Faraday Version is greater than
2 because Faraday V1 includes the gem already.

alexrudall#392
@ajGingrich ajGingrich force-pushed the only-require-faraday-multipart-for-faraday-2 branch from 85a63ef to 4201896 Compare December 1, 2023 18:08
@alexrudall
Copy link
Owner

Cool, thank you @ajGingrich! Have you tested this does work with both versions of Faraday?

@ajGingrich
Copy link
Author

@alexrudall I haven't explicitly tested it with the newer versions but I've confirmed that it works with Faraday 1.1. I also grabbed this change from a OpenAPI generated repo that works.

Here are the changes from Faraday with 2.0.0 release indicate the dropping of the multipart middleware.

I'm confident it should be good but I can try to think through testing it with different versions more extensively if you would prefer before merging!

@alexrudall
Copy link
Owner

@ajGingrich I would definitely prefer that as it could affect tens of thousands of people if we get it wrong :) I always try to test PRs as much as possible even small ones. Thank you!

@ajGingrich
Copy link
Author

@alexrudall Are you sure that Faraday 1 is supported? I was just testing a little and I'm getting failures that are unrelated to the multipart change.

Reproduction Steps

  • Checked out main
  • modified .gemspec to specifically require 1.1.0
spec.add_dependency "faraday", "~> 1.1.0"
  • docker-compose up
  • bundle
  • bundle exec rspec

I'm getting a number of failures but everything passes with 2.7. Perhaps the solution is to actually remove support for Faraday 1.

@alexrudall
Copy link
Owner

eep, you're right...

I think we need to require faraday json response and request middleware, but can't find the right library 🤔

Thanks for checking, that's a really good spot

@alexrudall
Copy link
Owner

If we can fix this, could be nice to add Faraday 1 + 2 to the CircleCI matrix so both get tested.

@ajGingrich ajGingrich force-pushed the only-require-faraday-multipart-for-faraday-2 branch from bd1ce31 to ddfb881 Compare December 10, 2023 21:54
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

Successfully merging this pull request may close these issues.

2 participants