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

Upgrade all packages using pur #4318

Merged
merged 10 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@humitos
Member

humitos commented Jul 2, 2018

As usual, upgrade all python packages from all requirements.

Just to know where we are regarding package versions and consider upgrade them if all test and manual QA pass.

for reqs in `ls requirements`;
  do pur --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth --requirement requirements/$reqs; 
done
Show outdated Hide outdated requirements/deploy.txt Outdated
Show outdated Hide outdated requirements/testing.txt Outdated
Show outdated Hide outdated requirements/testing.txt Outdated

@agjohnson agjohnson added this to the Cleanup milestone Jul 9, 2018

Show outdated Hide outdated requirements/pip.txt Outdated

humitos added some commits Aug 30, 2018

Upgrade all packages using pur
for reqs in `ls requirements`;
do
  pur
  --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth,django-filter,mercurial
  --requirement requirements/$reqs;
done
no-else-return fixed in all the files
Some block needed to be re-idented to fulfill this check.

Basically, if you have a "if something: return another" the "else"
clause is not needed since it will be the common case for the flow to
continue if the condition is not met.

@humitos humitos requested a review from rtfd/core Aug 30, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 30, 2018

Member

This PR is ready for review after some cleanup and fixes.

Member

humitos commented Aug 30, 2018

This PR is ready for review after some cleanup and fixes.

Show outdated Hide outdated readthedocs/wsgi.py Outdated
@@ -47,6 +51,9 @@ dnspython==1.15.0
# VCS
httplib2==0.11.3
# GitPython 2.1.11 makes TestGitBackend.test_git_tags to fail because
# of an UnicodeError

This comment has been minimized.

@stsewd

stsewd Aug 30, 2018

Member

Maybe mention that we can update when we drop py2 support

@stsewd

stsewd Aug 30, 2018

Member

Maybe mention that we can update when we drop py2 support

This comment has been minimized.

@humitos

humitos Aug 30, 2018

Member

I'm starting to feel that this package is too unstable to rely all our building process on it. We just updated a bugfix version and it breaks our flow.

In the requirements, it says that it supports 2.7 or newer: https://gitpython.readthedocs.io/en/stable/intro.html#requirements

This is the commit that we think that introduced the issue: gitpython-developers/GitPython@7f08b77

@humitos

humitos Aug 30, 2018

Member

I'm starting to feel that this package is too unstable to rely all our building process on it. We just updated a bugfix version and it breaks our flow.

In the requirements, it says that it supports 2.7 or newer: https://gitpython.readthedocs.io/en/stable/intro.html#requirements

This is the commit that we think that introduced the issue: gitpython-developers/GitPython@7f08b77

This comment has been minimized.

@humitos

humitos Aug 30, 2018

Member

More examples:

I suppose that we should start thinking about another solution, unfortunately.

@humitos

humitos Aug 30, 2018

Member

More examples:

I suppose that we should start thinking about another solution, unfortunately.

This comment has been minimized.

@humitos

humitos Aug 30, 2018

Member

This comment has been minimized.

@humitos

humitos Aug 30, 2018

Member

Tests in master and 2.1.11 fail. Also in 2.1.10... 😥

@humitos

humitos Aug 30, 2018

Member

Tests in master and 2.1.11 fail. Also in 2.1.10... 😥

This comment has been minimized.

@agjohnson

agjohnson Sep 4, 2018

Contributor

Regarding dropping py27 support, this will be our v3.0 -- it might make sense to time 3.0 for all of our deprecations in jan 2019. i opened #4594 to discuss more

@agjohnson

agjohnson Sep 4, 2018

Contributor

Regarding dropping py27 support, this will be our v3.0 -- it might make sense to time 3.0 for all of our deprecations in jan 2019. i opened #4594 to discuss more

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Sep 4, 2018

Member

This is ready for re-review and merge in my opinion.

Besides upgrading the packages, since some linting packages were updated, I fixed some new lint issues that appeared here (thanks to @stsewd!)

I'd like to merge this soon to avoid merge conflicts.

Member

humitos commented Sep 4, 2018

This is ready for re-review and merge in my opinion.

Besides upgrading the packages, since some linting packages were updated, I fixed some new lint issues that appeared here (thanks to @stsewd!)

I'd like to merge this soon to avoid merge conflicts.

@agjohnson

LGTM! This is another that we'll want to simmer a little. We'll want to be careful with some of this releasing to prod, and probably warrants some QA.

@@ -8,7 +8,7 @@
# This application object is used by any WSGI server configured to use this
# file. This includes Django's development server, if the WSGI_APPLICATION
# setting points here.
from django.core.wsgi import get_wsgi_application # pylint: disable=wrong-import-position
from django.core.wsgi import get_wsgi_application # noqa

This comment has been minimized.

@agjohnson

agjohnson Sep 5, 2018

Contributor

I do generally prefer explicit disables, though I know that prospector was the one making this easier. For example, prospetor knows that pylint and flake8 (or whatever) throw an error on the same line, but pylint is ignoring -- it will therefore ignore the flake8 error as well. I don't think our pre-commit pass has anything comparable unfortunately.

#noqa becomes a generic exception handler of sorts, where who knows what else we're ignoring when we use #noqa

@agjohnson

agjohnson Sep 5, 2018

Contributor

I do generally prefer explicit disables, though I know that prospector was the one making this easier. For example, prospetor knows that pylint and flake8 (or whatever) throw an error on the same line, but pylint is ignoring -- it will therefore ignore the flake8 error as well. I don't think our pre-commit pass has anything comparable unfortunately.

#noqa becomes a generic exception handler of sorts, where who knows what else we're ignoring when we use #noqa

This comment has been minimized.

@agjohnson

agjohnson Sep 5, 2018

Contributor

Not sure if this is possible. worth making an issue on common?

@agjohnson

agjohnson Sep 5, 2018

Contributor

Not sure if this is possible. worth making an issue on common?

This comment has been minimized.

@humitos

humitos Sep 5, 2018

Member

I also like being explicit here. I didn't find how to avoid the checking that was failing, though. It was something related to the import not at the top of the file and it wasn't pylint (I think it was flake --don't remember). That's why I just added # noqa.

@humitos

humitos Sep 5, 2018

Member

I also like being explicit here. I didn't find how to avoid the checking that was failing, though. It was something related to the import not at the top of the file and it wasn't pylint (I think it was flake --don't remember). That's why I just added # noqa.

@agjohnson agjohnson merged commit 8b7c279 into master Sep 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/pur/upgrade branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment