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

remove references to deprecated ::Parts and ::UploadIO #6

Merged

Conversation

jsvd
Copy link
Contributor

@jsvd jsvd commented Jun 6, 2022

Here's my attempt at bringing faraday-multipart up to par with multipart-post recent class reorganization changes.
This PR:

  • changes class references according to Rework file structure & license header. socketry/multipart-post#89
    • ::UploadIO to ::Multipart::Post::UploadIO
    • ::Parts to ::Multipart::Post::Parts
  • replaces fine grained requires with require 'multipart/post' as per recommendation
  • bumps this gem to the next major version as it now has a lower bound requirement of 'multipart-post', '>= 2.2.0' (where previously it was >= 1.2. This change can be made to a minor or patch version bump instead, up to the maintainer.

fixes #5

@olleolleolle
Copy link
Member

Thanks for the PR!

I'd like to see if we were able to get this working with both the versions before the change in structure and after. Would it be a lot of changes to this PR?

@olleolleolle
Copy link
Member

(And, as a follow-up, don't include changes to the gem version in this PR, it's a later decision what version number to use. We change that during the release process.)

@jsvd
Copy link
Contributor Author

jsvd commented Jun 6, 2022

I'd like to see if we were able to get this working with both the versions before the change in structure and after. Would it be a lot of changes to this PR?

Not a lot, do you prefer to achieve bwc through detecting the multipart-post gem version (from version.rb) or through defined? ?

(And, as a follow-up, don't include changes to the gem version in this PR, it's a later decision what version number to use. We change that during the release process.)

Will do.

@olleolleolle
Copy link
Member

@jsvd In this case, I'd prefer the very explicit version comparison! Thanks for asking.

Copy link

@mashhurs mashhurs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

faraday-multipart.gemspec Outdated Show resolved Hide resolved
@jsvd
Copy link
Contributor Author

jsvd commented Jun 6, 2022

@olleolleolle let me know what you think. Because multipart-post 2.1 has a different file structure than 2.2, even checking the gem's VERSION can cause deprecation warnings. This is why I resorted to doing require + begin/rescue.

CHANGELOG.md Outdated Show resolved Hide resolved
@olleolleolle
Copy link
Member

@jsvd Thanks for keeping at it! This is a tricky thing.

@jsvd jsvd requested a review from olleolleolle June 7, 2022 08:46
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps forward!

lib/faraday/multipart/version.rb Show resolved Hide resolved
@ioquatix
Copy link

ioquatix commented Jun 7, 2022

Thanks for your patient, thoughtful and thorough work.

faraday-multipart.gemspec Outdated Show resolved Hide resolved
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks 👍 and I thank you, @jsvd!

@olleolleolle olleolleolle merged commit 2127a0c into lostisland:main Jun 7, 2022
@jsvd jsvd deleted the update_multipart_post_class_references branch June 7, 2022 12:22
@olleolleolle
Copy link
Member

This has now been released as https://rubygems.org/gems/faraday-multipart/versions/1.0.4

@@ -31,5 +31,5 @@ Gem::Specification.new do |spec|

spec.required_ruby_version = '>= 2.4', '< 4'

spec.add_dependency 'multipart-post', '>= 1.2', '< 3'
spec.add_dependency 'multipart-post', '~> 2'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to declare a dependency on any future version of multipart-post? Presumably 3.0, 4.0, 17.0, etc will likely have breaking changes, but this version constraint is essentially >= 2.0. If you want to to depend on any 2.x version this should probably be ~> 2.0.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd advise to use ~> 2.0.

Copy link
Member

@olleolleolle olleolleolle Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! a3609a6 now uses that.

(👋 Hi, everyone!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unnecessary as ~> 2 is equivalent to ~> 2.0:

irb(main):001:0> Gem::Requirement.new('~> 2').satisfied_by? Gem::Version.new("2.0")
=> true
irb(main):002:0> Gem::Requirement.new('~> 2').satisfied_by? Gem::Version.new("2.2")
=> true
irb(main):003:0> Gem::Requirement.new('~> 2').satisfied_by? Gem::Version.new("1")
=> false
irb(main):004:0> Gem::Requirement.new('~> 2').satisfied_by? Gem::Version.new("3.0")
=> false
irb(main):005:0> Gem::Requirement.new('~> 2').satisfied_by? Gem::Version.new("4.0")
=> false

From Rubygem docs https://guides.rubygems.org/patterns/:

It’s also important to know that if you specify a major version only, like this:

spec.add_runtime_dependency 'library', '~> 2'

It will only use the latest version from the 2.x series – so 2.3.0 – and not 3.0.0. This behaviour may surprise some people, but automatically allowing any major version past version 2 is more surprising behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jsvd, for details!

(The change in the gemspec is now "the less surprising one".)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsvd thanks for teaching me something new! I didn't realize rubygems had a special case for a single digit...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know it either! Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! Thank you @jsvd 🙏

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.

Use new multipart-post 2.2.0 release
6 participants