-
Notifications
You must be signed in to change notification settings - Fork 4
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
Python support update #6
Conversation
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.
LGTM - with some comments. Please try to apply the suggested changes.
9b564fa
to
18e1ff4
Compare
Hi @sgaist currently, CI is broken, see https://travis-ci.org/github/morepath/more.webassets/builds/731987458 The reason seems that Travis nowadays need the "matrix" syntax for configuring several Python interpreters, also see here As a side note .. I just saw you force-pushed some new changes. While this works, there is one problem with this. Above I added some inline annotations on what needs to be done for this pr - as you force pushed a new state, these do not match any more. On the other hand, if you just added another commit, I could easily see if you applied the requested changes. Do not worry about too many commits, as the maintainer always can squash all changes into one commit on merging. So, the rule of thumb I follow - when I work together with other people, especially via GitHub, I do not force push. Thanks for contributing! |
@jugmac00 I see your point, and I will do it like that. I forced push because I am used to provide sets of commits (even if squashed) that are "clean" in the sense that the kind of small fixes you asked were better served by being integrated in the original work. |
The old env entry as is breaks the build
I absolutely noticed that your commits are super clean! 👍 |
One last thing - could you please add a change log entry? Thank you! |
Thank you! |
This merge request updates the minimal Python version to 3.7 and adds 3.8.
It also adds the tools required to run the tests to list of dependencies.
Fixes #5