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.

Sign up for GitHub

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

Jump to bottom

Read Content type from cached and uploaded file. #1245

Merged
merged 1 commit into from
Oct 15, 2013
Merged

Read Content type from cached and uploaded file. #1245

merged 1 commit into from
Oct 15, 2013

Conversation

klacointe
Copy link
Contributor

Rework Pull Request #1216 into a single commit.

@klacointe klacointe mentioned this pull request Oct 15, 2013
Closed
@thiagofm
Copy link
Member

I like this solution.

@bensie
Copy link
Member

bensie commented Oct 15, 2013

What about deprecating the old behavior? At this point anyone using the existing module will get a LoadError when mixing it in to their uploader.

@klacointe
Copy link
Contributor Author

We can re-add the MimeTypes module and puts a Deprecation warning but it seems useless…
I prefer, in this case, an upgrading notes in the README, but it's my opinion.
Would you like me to choose the deprecation warning solution ?

@bensie
Copy link
Member

bensie commented Oct 15, 2013

Yes, we definitely need to deprecate for one 0.x release. We're not yet held to sem-ver in pre-1.0, but I don't want to needlessly break stuff. This should be in addition to upgrade notes in the readme/wiki.

@thiagofm
Copy link
Member

I'm with @bensie for the deprecation warning.

@klacointe
Copy link
Contributor Author

I rewrote my commit to keep MimeTypes module and puts a deprecation warning if someone use it.
democracy won 😤

bensie
bensie reviewed Oct 15, 2013
View reviewed changes
lib/carrierwave/processing/mime_types.rb
@@ -27,6 +27,7 @@ module MimeTypes
extend ActiveSupport::Concern

included do
puts "WARNING: Carrierwave::MimeTypes is deprecated"
Copy link
Member

Choose a reason for hiding this comment

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

Let's use ActiveSupport::Deprecation for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but i don't know the "deprecation horizon" version.

@klacointe
Copy link
Contributor Author

I've set the Mime::Types deprecation to 1.0, but feel free to update the version.

bensie
bensie reviewed Oct 15, 2013
View reviewed changes
lib/carrierwave/processing/mime_types.rb
@@ -27,6 +27,7 @@ module MimeTypes
extend ActiveSupport::Concern

included do
ActiveSupport::Deprecation.new "1.0", "Carrierwave::MimeTypes"
Copy link
Member

Choose a reason for hiding this comment

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

Add the missing require 'activesupport/deprecation' and set the horizon to 0.11.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.

Done, s/activesupport/active_support

@klacointe
Copy link
Contributor Author

@bensie thanks for the review 😘

@bensie
Copy link
Member

bensie commented Oct 15, 2013

Thanks for cranking out the changes so quick!

bensie
bensie reviewed Oct 15, 2013
View reviewed changes
lib/carrierwave/uploader/proxy.rb
# [String] content type of the file
#
def content_type
file.try(:content_type)
Copy link
Member

Choose a reason for hiding this comment

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

Probably should just use respond_to? here -- if you really want Object#try you need to require "active_support/core_ext/object"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Object#try used in another place in carrierwave ? If not, using respond_to? seems better.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let's use respond_to?

bensie added a commit that referenced this pull request Oct 15, 2013
@bensie
Merge pull request #1245 from klacointe/content_type
6f15bde 
Read Content type from cached and uploaded file.
@bensie bensie merged commit 6f15bde into carrierwaveuploader:master Oct 15, 2013
@bensie
Copy link
Member

bensie commented Oct 15, 2013

Good to go -- thanks!

@klacointe
Copy link
Contributor Author

😤

@chikamichi
Copy link

So nice. 💎

@bensie
Copy link
Member

bensie commented Oct 15, 2013

Uh oh -- we might not be there just yet...
https://travis-ci.org/carrierwaveuploader/carrierwave/jobs/12590492

@klacointe
Copy link
Contributor Author

@bensie we should use warn method for activesupport < 4.0
http://rdoc.info/docs/rails/3.2.8/ActiveSupport/Deprecation#warn-class_method

@klacointe
Copy link
Contributor Author

@bensie #1246

@taavo
Copy link
Member

taavo commented Oct 16, 2013

Thanks for stepping in and making this happen @bensie!

mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Dec 31, 2015
Remove MimeTypes module
8b6df15 
Since carrierwaveuploader#1245, CarrierWave::MimeTypes module is redundant as `mime-types`
gem is a runtime dependency now, and `SanitizedFile` gets it's
content_type using it.
The module was deprecated since 0.10 and warned developers when used it.
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
Remove MimeTypes module
5e596c9 
Since carrierwaveuploader#1245, CarrierWave::MimeTypes module is redundant as `mime-types`
gem is a runtime dependency now, and `SanitizedFile` gets it's
content_type using it.
The module was deprecated since 0.10 and warned developers when used it.
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
Remove CarrierWave::MimeTypes processor module
b80b8a3 
Since carrierwaveuploader#1245 (*), CarrierWave::MimeTypes module is redundant as
`mime-types` gem is a runtime dependency now, and `SanitizedFile` gets
it's content_type using it.
The module was deprecated since 0.10 and warned developers when used it.

(*) I discovered this when tried to fix specs marked as pending. I
looked at commits history and PRs to understand what happened.
It started with carrierwaveuploader#372 that added `CarrierWave::MimeTypes`  processor (Jun
2011), then carrierwaveuploader#1216 and carrierwaveuploader#1245 that made `mime-types` a runtime dependency
and deprecated `CarrierWave::MimeTypes` module (2013), and finally carrierwaveuploader#1584
`CarrierWave::MagicMimeTypes` (2015)
@mehlah mehlah mentioned this pull request Jan 1, 2016
Merged
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
Remove CarrierWave::MimeTypes processor module
2fc9ed3 
Since carrierwaveuploader#1245 (*), CarrierWave::MimeTypes module is redundant as
`mime-types` gem is a runtime dependency now, and `SanitizedFile` gets
it's content_type using it.
The module was deprecated since 0.10 and warned developers when used it.

(*) I discovered this when tried to fix specs marked as pending. I
looked at commits history and PRs to understand what happened.
It started with carrierwaveuploader#372 that added `CarrierWave::MimeTypes`  processor (Jun
2011), then carrierwaveuploader#1216 and carrierwaveuploader#1245 that made `mime-types` a runtime dependency
and deprecated `CarrierWave::MimeTypes` module (2013), and finally carrierwaveuploader#1584
`CarrierWave::MagicMimeTypes` (2015)
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
Remove CarrierWave::MimeTypes processor module
947eee5 
Since carrierwaveuploader#1245 (*), CarrierWave::MimeTypes module is redundant as
`mime-types` gem is a runtime dependency now, and `SanitizedFile` gets
it's content_type using it.
The module was deprecated since 0.10 and warned developers when used it.

(*) I discovered this when tried to fix specs marked as pending. I
looked at commits history and PRs to understand what happened.
It started with carrierwaveuploader#372 that added `CarrierWave::MimeTypes`  processor (Jun
2011), then carrierwaveuploader#1216 and carrierwaveuploader#1245 that made `mime-types` a runtime dependency
and deprecated `CarrierWave::MimeTypes` module (2013), and finally carrierwaveuploader#1584
`CarrierWave::MagicMimeTypes` (2015)
mynock pushed a commit to mynock/carrierwave that referenced this pull request Aug 9, 2016
Remove CarrierWave::MimeTypes processor module
71d4941 
Since carrierwaveuploader#1245 (*), CarrierWave::MimeTypes module is redundant as
`mime-types` gem is a runtime dependency now, and `SanitizedFile` gets
it's content_type using it.
The module was deprecated since 0.10 and warned developers when used it.

(*) I discovered this when tried to fix specs marked as pending. I
looked at commits history and PRs to understand what happened.
It started with carrierwaveuploader#372 that added `CarrierWave::MimeTypes`  processor (Jun
2011), then carrierwaveuploader#1216 and carrierwaveuploader#1245 that made `mime-types` a runtime dependency
and deprecated `CarrierWave::MimeTypes` module (2013), and finally carrierwaveuploader#1584
`CarrierWave::MagicMimeTypes` (2015)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@klacointe @thiagofm @bensie @chikamichi @taavo