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

Detect Content-Type when content_type property is null #407

Merged
merged 3 commits into from
Mar 1, 2018

Conversation

bxm156
Copy link
Contributor

@bxm156 bxm156 commented Oct 14, 2017

Fallback on other content-type detection methods when the content_type property is None
Fixes #406

@bxm156
Copy link
Contributor Author

bxm156 commented Oct 14, 2017

The failure is unrelated to my changes as far as I can tell. Perhaps a cache issue with pip on Travis CI?

@codecov-io
Copy link

codecov-io commented Oct 20, 2017

Codecov Report

Merging #407 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
+ Coverage   76.05%   76.08%   +0.03%     
==========================================
  Files          11       11              
  Lines        1566     1568       +2     
==========================================
+ Hits         1191     1193       +2     
  Misses        375      375
Impacted Files Coverage Δ
storages/backends/s3boto3.py 86.58% <100%> (+0.04%) ⬆️
storages/backends/s3boto.py 87.45% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adefe32...2594683. Read the comment docs.

@bxm156
Copy link
Contributor Author

bxm156 commented Oct 20, 2017

@jschneier Updated with a fix for failing tests on master.

paramiko now requires the enum library, which wasn't added to Python 3 until Python 3.4. I updated tox to include the dependency for python 3.3 tests.

Copy link
Contributor

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell!

@alexhayes
Copy link

Works for me also - thanks @bxm156

@Alir3z4
Copy link

Alir3z4 commented Dec 1, 2017

This pull request fix the issue I'm having too.
Any idea what is left to get it merged into master ?

ipmb added a commit to lincolnloop/django-cms-demo that referenced this pull request Feb 10, 2018
@AnderUstarroz
Copy link

@jschneier this commit fixes an issue that is giving headaches to many developers, any chance to get it merged into master?

Well done @bxm156

@bxm156
Copy link
Contributor Author

bxm156 commented Mar 1, 2018

Perhaps @jleclanche could help get this merged?

@jleclanche jleclanche merged commit 95e49e0 into jschneier:master Mar 1, 2018
@jleclanche
Copy link
Contributor

Sure. I can't make releases though.

@bhrutledge
Copy link

This resolves an issue plaguing Django CMS users. @jschneier Any thoughts on a release?

@jschneier
Copy link
Owner

Thanks for the ping, sure. @jleclanche thanks for the assist on the maintenance. I'm close to getting to the point where I can spend 1-2 full days/month on open source.

@jleclanche
Copy link
Contributor

@jschneier Have you thought about moving django-storages to jazzband? I've done it for one of my django projects and it's a huge weight off my mind

@jschneier
Copy link
Owner

jschneier commented Mar 26, 2018 via email

@jleclanche
Copy link
Contributor

@jschneier Worth giving it another shot. No project's perfect, but this one is used by a lot of people, and at the end of the day reducing bus factor is the whole goal.

@jschneier
Copy link
Owner

jschneier commented Mar 26, 2018 via email

@bxm156
Copy link
Contributor Author

bxm156 commented Mar 26, 2018

Thanks @jschneier!

Side Question:
What work do you think is needed in order to make this project viable for something like Jazzband?

@jschneier
Copy link
Owner

jschneier commented Mar 27, 2018 via email

@jleclanche
Copy link
Contributor

FWIW I don't think any of those things are required to get into jazzband. Ci and an easy to run test suite so that pull requests can be merged without too deep an understanding of the codebase is what's important.

Regarding testing with real credentials, it's certainly nice to have the possibility, but in CI I recommend "real" mock implementations. Either stuff like moto or minio for S3, for example.

@jschneier
Copy link
Owner

jschneier commented Mar 27, 2018 via email

@bhrutledge
Copy link

@jschneier Thank you for the prompt response! It was a pleasant surprise.

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.

9 participants