Skip to content
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

All package updates #4792

Merged
merged 10 commits into from
Nov 29, 2018
Merged

All package updates #4792

merged 10 commits into from
Nov 29, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Oct 22, 2018

$ for reqs in `ls requirements`; do pur --skip django,docker,celery,commonmark,gitpython,elasticsearch,pyelasticsearch,mercurial --requirement requirements/$reqs; done

Closes #4897

@humitos humitos requested a review from a team October 22, 2018 16:30
@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

Merging #4792 into master will increase coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #4792      +/-   ##
==========================================
+ Coverage   76.75%   76.75%   +<.01%     
==========================================
  Files         158      158              
  Lines       10048    10051       +3     
  Branches     1265     1265              
==========================================
+ Hits         7712     7715       +3     
  Misses       1995     1995              
  Partials      341      341
Impacted Files Coverage Δ
readthedocs/search/utils.py 0% <0%> (ø) ⬆️
readthedocs/restapi/views/integrations.py 88.65% <100%> (ø) ⬆️
readthedocs/restapi/views/model_views.py 94.18% <100%> (+0.1%) ⬆️
readthedocs/projects/constants.py 100% <100%> (ø) ⬆️

@stsewd
Copy link
Member

stsewd commented Oct 26, 2018

@humitos now that we are on a recent version of django, we can revert the last commit p: or maybe upgrade the rest of packages too!

@humitos
Copy link
Member Author

humitos commented Oct 27, 2018

@stsewd thanks for noting this.

I've updated all the packages and followed the migration guides that we had noted in the requirements files. I think that all tests should pass.

@humitos humitos added the PR: work in progress Pull request is not ready for full review label Oct 27, 2018
# Besides the guide, check this comment for the migration
# https://github.com/rtfd/readthedocs.org/pull/4318#discussion_r214163531
django-filter==1.1.0
django-filter==2.0.0
Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to upgrade to django-filter>=2.0.0 we need to drop support for Python2 since it requires Python3. Tests fail with:

>   from .filterset import FilterSet
E     File "/home/travis/build/rtfd/readthedocs.org/.tox/py27/lib/python2.7/site-packages/django_filters/filterset.py", line 184
E       def __init__(self, data=None, queryset=None, *, request=None, prefix=None):
E                                                     ^
E   SyntaxError: invalid syntax

@humitos humitos requested a review from a team November 8, 2018 09:49
@humitos humitos removed the PR: work in progress Pull request is not ready for full review label Nov 8, 2018
@@ -2,8 +2,8 @@
# http://initd.org/psycopg/docs/install.html#binary-install-from-pypi
psycopg2==2.7.5 --no-binary psycopg2
gunicorn==19.9.0
pysolr==3.7.0
django-redis-cache==1.7.1
pysolr==3.8.1
Copy link
Member

Choose a reason for hiding this comment

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

This looks like something used for hystack only https://github.com/django-haystack/pysolr, it was removed a time ago #4039

# Required to avoid Transifex error with reserved slug
# https://github.com/sphinx-doc/sphinx-intl/pull/27
git+https://github.com/agjohnson/sphinx-intl.git@7b5c66bdb30f872b3b1286e371f569c8dcb66de5#egg=sphinx-intl

Pygments==2.2.0

mkdocs==0.17.3
mkdocs==1.0.4
# mkdocs requires markdown
# Markdown 3.0 breaks with older Django Rest Framework
Markdown<3.0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we are safe to update this?

# E def __init__(self, data=None, queryset=None, *, request=None, prefix=None):
# E ^
# E SyntaxError: invalid syntax
django-filter<2.0.0
Copy link
Member

Choose a reason for hiding this comment

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

I guess the above comments

filter_fields = ('slug',)  # django-filter<2.0.0
filterset_fields = ('slug',)

are because of this? I think is better to just create an issue, rather than hiding it on the code

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't upgrade to 2.0.0 yet because we are still running tests with Py2. Once we remove that, we can upgrade without touching the code.

The comments added into the code already prepared it for 2.0.0.

Copy link
Member Author

@humitos humitos Nov 9, 2018

Choose a reason for hiding this comment

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

I prefer to prepare the code to work with both versions now and when we upgrade, we can remove the lines that are not needed.

$ for reqs in `ls requirements`; do pur --skip django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,mkdocs,gitpython,mercurial --requirement requirements/$reqs; done
$ for reqs in `ls requirements`; do pur --skip django,docker,celery,gitpython,elasticsearch,pyelasticsearch,mercurial --requirement requirements/$reqs; done
$ for reqs in `ls requirements`; do pur --skip stripe,commonmark,django,docker,celery,gitpython,elasticsearch,pyelasticsearch,mercurial --requirement requirements/$reqs; done
$ for reqs in `ls requirements`; do pur --skip django,docker,celery,commonmark,gitpython,elasticsearch,pyelasticsearch,mercurial --requirement requirements/$reqs; done
@humitos humitos requested a review from a team November 29, 2018 20:22
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think we should merge this and update locally to test it before it gets deployed.

@ericholscher ericholscher merged commit b15e5d3 into master Nov 29, 2018
@humitos humitos deleted the humitos/pur/update branch November 29, 2018 20:30
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