Skip to content

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

2 participants