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

Use Marcel::Magic.new(content_type).extensions instead of Marcel::TYPES[content_type] #2728

Merged
merged 4 commits into from Mar 2, 2024

Conversation

schinery
Copy link
Contributor

@schinery schinery commented Feb 29, 2024

This commit of the Marcel gem, which has just shipped in version 1.0.3, breaks Carrierwave as the Marcel::TYPES constant used here has been removed.

This PR is to switch to using Marcel::Magic.new(content_type).extensions to return the extensions and not use constants.

@schinery schinery marked this pull request as draft February 29, 2024 14:48
@schinery schinery marked this pull request as ready for review February 29, 2024 14:49
@schinery schinery marked this pull request as draft February 29, 2024 14:54
@schinery
Copy link
Contributor Author

Not sure why, but this PR is getting the following failure in the ruby-head tests for ./spec/processing/rmagick_spec.rb:248.

expected NoMethodError with message matching /private method `foo=' called/, got #<NoMethodError: private method 'foo=' called for an instance of Magick::Image::Info> with backtrace:

Doesn't seem to be related to these changes, but flagging anyway.

@BrianHawley
Copy link
Contributor

BrianHawley commented Mar 1, 2024

@schinery the test failure in ruby-head is because of https://bugs.ruby-lang.org/issues/16495

In spec/processing/rmagick_spec.rb line 258, change:

/private method `foo=' called/

to:

/private method .foo=. called/

That will handle the quoting change.

@schinery
Copy link
Contributor Author

schinery commented Mar 1, 2024

@BrianHawley that did the trick 👍🏻

@bvogel
Copy link

bvogel commented Mar 1, 2024

I would vote not not move from one private constant to another

mime_type = Marcel::TYPES[content_type]
unless File.extname(filename).present? || mime_type.blank?
extension = mime_type[0].first
extensions = Marcel::TYPE_EXTS[content_type]
Copy link

Choose a reason for hiding this comment

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

Suggested change
extensions = Marcel::TYPE_EXTS[content_type]
extensions = Marcel::Magic.new(content_type).extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is much nicer 😄

@schinery schinery requested a review from bvogel March 1, 2024 17:39
@schinery schinery changed the title Use Marcel::TYPE_EXTS instead of Marcel::TYPES Use Marcel::Magic.new(content_type).extensions instead of Marcel::TYPES[content_type] Mar 1, 2024
@@ -23,7 +23,7 @@ Gem::Specification.new do |s|
s.add_dependency "activesupport", ">= 6.0.0"
s.add_dependency "activemodel", ">= 6.0.0"
s.add_dependency "image_processing", "~> 1.1"
s.add_dependency "marcel", "~> 1.0.0"
s.add_dependency "marcel", "~> 1.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

With the new code change, you don't need to make this version change, because the code is supported as far back as version 1.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this change 👍🏻

@bvogel
Copy link

bvogel commented Mar 1, 2024

Nice work everyone!

@mshibuya mshibuya merged commit e148cd8 into carrierwaveuploader:master Mar 2, 2024
12 checks passed
@mshibuya
Copy link
Member

mshibuya commented Mar 2, 2024

Thank you for the fix!

mshibuya added a commit that referenced this pull request Mar 2, 2024
Use Marcel::Magic.new(content_type).extensions instead of Marcel::TYPES[content_type]
@bvogel
Copy link

bvogel commented Mar 2, 2024

@mshibuya any chance for a new release soon?

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.

None yet

4 participants