- Notifications
You must be signed in to change notification settings - Fork294
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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=trueAnd then in |
@jayvdb It appears the minimum required version is actually1.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 toSIX_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. |
Uh oh!
There was an error while loading.Please reload this page.
This project uses a syntax for importing urllib.parse from six which was introduced in Six version 1.4
The import can be foundhere
The relevant change in Six can be foundhere
This can cause an issue if a project has a previous version of Six (say version 1.3) installed when installing html5lib
EDIT:
importing
viewkeysin html5parser.py requires six 1.9