Skip to content

/ html5lib-python

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

Jump to bottom

Declare implicit dependency on Six 1.9 or higher #301

Merged
merged 1 commit into from Oct 27, 2016
Merged

Declare implicit dependency on Six 1.9 or higher #301

merged 1 commit into from Oct 27, 2016

Conversation

@amorde
Copy link
Contributor

@amorde amorde commented Sep 15, 2016 •
edited

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
@jayvdb
jayvdb approved these changes Sep 15, 2016
View changes
@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Sep 15, 2016

I need this for #294.
@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Sep 15, 2016

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.
@amorde
Copy link
Contributor Author

@amorde amorde commented Sep 15, 2016

@jayvdb That sounds like a great idea, I'm not really sure how to go about doing that though.
@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Sep 15, 2016

You could add a job with

 env:
   - USE_OPTIONAL=true
   - USE_OPTIONAL=false
+  - USE_SIX_14=true

And then in requirements-install.sh look for that flag and force install 1.4 (and if anything upgrades it to a more recent version, fix it).
@amorde
Copy link
Contributor Author

@amorde amorde commented Sep 26, 2016

@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
@amorde amorde changed the title Declare implicit dependency on Six 1.4 or higher Declare implicit dependency on Six 1.9 or higher Sep 26, 2016
@jayvdb jayvdb mentioned this pull request Sep 27, 2016
Closed
@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Sep 27, 2016

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.
@jayvdb
jayvdb suggested changes Sep 27, 2016
View changes
Copy link
Contributor

@jayvdb jayvdb left a comment

Unnecessary jobs created.
@jayvdb
jayvdb approved these changes Sep 29, 2016
View changes
Copy link
Contributor

@jayvdb jayvdb left a comment

The commits probably need squashing.
.travis.yml Outdated
@@ -16,6 +16,7 @@ cache:
env:
- USE_OPTIONAL=true
- USE_OPTIONAL=false
- USE_SIX_LOWEST_VERSION=1.9 USE_OPTIONAL=true

This comment has been minimized.

Sign in to view
@jayvdb

jayvdb Sep 29, 2016
Contributor

USE_SIX_LOWEST_VERSION is very long. Could be shorter.

This comment has been minimized.

Sign in to view
@amorde

amorde Sep 30, 2016
Author Contributor

Changed to SIX_VERSION
@amorde
Declare explicit dependency on Six 1.9
Loading status checks…
fa6405b
@jayvdb
jayvdb approved these changes Sep 30, 2016
View changes
@gsnedders gsnedders mentioned this pull request Oct 6, 2016
Closed
@amorde
Copy link
Contributor Author

@amorde amorde commented Oct 18, 2016

Hey @jayvdb, any updates on this?
@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Oct 18, 2016

@amorde , I am not a committer here. I can only do code review, which I have done.
I am also waiting for this to be reviewed/merged so that I can complete #294. ;-)
@amorde
Copy link
Contributor Author

@amorde amorde commented Oct 18, 2016

@jayvdb gotcha, haha sorry for the mixup!
@gsnedders
Copy link
Member

@gsnedders gsnedders commented Oct 27, 2016

Thanks for the reviewing, @jayvdb! :)

Ultimately, we should almost certainly declare (and test) minimum required versions for all our dependencies.
@gsnedders gsnedders merged commit ff6111c into html5lib:master Oct 27, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gsnedders gsnedders mentioned this pull request Oct 27, 2016
Open
@davidread davidread mentioned this pull request Mar 8, 2017
Closed
@Chronial Chronial mentioned this pull request Nov 10, 2017
Closed
@gsnedders gsnedders mentioned this pull request Nov 28, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayvdb jayvdb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@amorde @jayvdb @gsnedders
You can’t perform that action at this time.