-
Notifications
You must be signed in to change notification settings - Fork 294
Declare implicit dependency on Six 1.9 or higher #301
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
Conversation
I need this for #294. |
It would be nice to have a test job that explicitly requires and uses six 1.4 , so that newer features are not inadvertently used without upgrading the requirement. |
@jayvdb That sounds like a great idea, I'm not really sure how to go about doing that though. |
You could add a job with env:
- USE_OPTIONAL=true
- USE_OPTIONAL=false
+ - USE_SIX_14=true And then in |
@jayvdb It appears the minimum required version is actually 1.9. Good call on the testing thing. I'll squash these commits and do a force push |
In addition to the Travis errors described in #298, you are introducing many additional Travis jobs per build. This problem only requires one additional job. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary jobs created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commits probably need squashing.
@@ -16,6 +16,7 @@ cache: | |||
env: | |||
- USE_OPTIONAL=true | |||
- USE_OPTIONAL=false | |||
- USE_SIX_LOWEST_VERSION=1.9 USE_OPTIONAL=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USE_SIX_LOWEST_VERSION
is very long. Could be shorter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to SIX_VERSION
Hey @jayvdb, any updates on this? |
@jayvdb gotcha, haha sorry for the mixup! |
Thanks for the reviewing, @jayvdb! :) Ultimately, we should almost certainly declare (and test) minimum required versions for all our dependencies. |
This project uses a syntax for importing urllib.parse from six which was introduced in Six version 1.4
The import can be found here
The relevant change in Six can be found here
This can cause an issue if a project has a previous version of Six (say version 1.3) installed when installing html5lib
EDIT:
importing
viewkeys
in html5parser.py requires six 1.9