Skip to content

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

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

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
Copy link
Contributor

jayvdb commented Sep 15, 2016

I need this for #294.

@jayvdb
Copy link
Contributor

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 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 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 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
Copy link
Contributor

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.

Copy link
Contributor

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

Unnecessary jobs created.

Copy link
Contributor

@jayvdb jayvdb left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to SIX_VERSION

@gsnedders gsnedders mentioned this pull request Oct 6, 2016
@amorde
Copy link
Contributor Author

amorde commented Oct 18, 2016

Hey @jayvdb, any updates on this?

@jayvdb
Copy link
Contributor

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 commented Oct 18, 2016

@jayvdb gotcha, haha sorry for the mixup!

@gsnedders
Copy link
Member

Thanks for the reviewing, @jayvdb! :)

Ultimately, we should almost certainly declare (and test) minimum required versions for all our dependencies.

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