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

chore: Vendor python-mimeparse #1439

Merged
merged 2 commits into from
Feb 12, 2019

Conversation

kgriffs
Copy link
Member

@kgriffs kgriffs commented Feb 12, 2019

Closes #1420

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #1439 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1439   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          39      39           
  Lines        2565    2565           
  Branches      396     396           
======================================
  Hits         2565    2565
Impacted Files Coverage Δ
falcon/request.py 100% <100%> (ø) ⬆️
falcon/media/handlers.py 100% <100%> (ø) ⬆️

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 14b482a...8e71cba. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #1439 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1439   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          39      39           
  Lines        2565    2565           
  Branches      396     396           
======================================
  Hits         2565    2565
Impacted Files Coverage Δ
falcon/request.py 100% <100%> (ø) ⬆️
falcon/media/handlers.py 100% <100%> (ø) ⬆️

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 f2d1a61...32a3993. Read the comment docs.

@kgriffs kgriffs force-pushed the 1420_vendor_mimeparse branch 5 times, most recently from 49c1217 to 17f4fc6 Compare February 12, 2019 06:15
@@ -0,0 +1,12 @@
#!/usr/bin/env bash

_EXPECTED_VERSION_MIMEPARSE="1.6.0"
Copy link
Member Author

@kgriffs kgriffs Feb 12, 2019

Choose a reason for hiding this comment

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

In the interest of KISS, the version number is hard-coded here instead of going to the trouble of reading it from mimeparse.py. We can always change this later.

Copy link
Member

@vytas7 vytas7 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 great, apart from newline nitpicks, I only had one issue when testing this briefly: it seemed that the vendored mimeparse would not cythonize [1]. Shall we list falcon.vendor in setup.py (haven't tried yet):

package_names = ['falcon', 'falcon.util', 'falcon.routing', 'falcon.media']
  1. A simple illustration
>>> import falcon
>>> falcon.api
<module 'falcon.api' from '/tmp/mimeparse-vendortest/lib/python3.5/site-packages/falcon/api.cpython-35m-x86_64-linux-gnu.so'>
>>> falcon.request
<module 'falcon.request' from '/tmp/mimeparse-vendortest/lib/python3.5/site-packages/falcon/request.cpython-35m-x86_64-linux-gnu.so'>
>>> falcon.vendor.mimeparse
<module 'falcon.vendor.mimeparse' from '/tmp/mimeparse-vendortest/lib/python3.5/site-packages/falcon/vendor/mimeparse/__init__.py'>
>>> falcon.vendor.mimeparse.mimeparse
<module 'falcon.vendor.mimeparse.mimeparse' from '/tmp/mimeparse-vendortest/lib/python3.5/site-packages/falcon/vendor/mimeparse/mimeparse.py'>


"""

from .mimeparse import * # NOQA
Copy link
Member

Choose a reason for hiding this comment

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

No new line at the end of file (in the Unix world, it is a punishable offense 😈 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Sheesh. Usually I add an extra line without thinking. Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Editors normally add that, at least for source files. And flake8 would complain, but NOQA on the last line felled us here. At least Github rendered it in a very visual way, making my job easy 🙂

fi

echo "Latest version of python-mimeparse is newer than expected."
exit 1
Copy link
Member

Choose a reason for hiding this comment

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

No new line at the end of file.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@kgriffs
Copy link
Member Author

kgriffs commented Feb 12, 2019

it seemed that the vendored mimeparse would not cythonize

Nice catch, fixed (and tested locally on my box).

@kgriffs kgriffs force-pushed the 1420_vendor_mimeparse branch 2 times, most recently from 51dbc4a to 65bfcff Compare February 12, 2019 08:29
@kgriffs kgriffs force-pushed the 1420_vendor_mimeparse branch from 65bfcff to faf6201 Compare February 12, 2019 08:30
Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

Looking good now!

Copy link
Member

@jmvrbanac jmvrbanac left a comment

Choose a reason for hiding this comment

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

lgtm

@jmvrbanac jmvrbanac merged commit f89b28d into falconry:master Feb 12, 2019
@jmvrbanac jmvrbanac deleted the 1420_vendor_mimeparse branch February 12, 2019 15:33
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.

3 participants