Try unpinning testing reqs #2863

agjohnson · 2017-05-22T18:02:12Z

No description provided.

ericholscher · 2017-05-24T21:30:22Z

readthedocs/integrations/models.py

@@ -305,7 +305,7 @@ def save(self, *args, **kwargs):
            if token is None:
                token = default_token()
                self.provider_data = {'token': token}
-        super(GenericAPIWebhook, self).save(*args, **kwargs)


Is there a reason we're removing *args? If someone passes in args, we don't want it to explode?

this should always be kwargs, but yeah, probably safest as *args, **kwargs

ericholscher · 2017-05-24T21:30:57Z

requirements/lint.txt

 prospector
-pylint-django<0.7
-pyflakes<1.2.0


Believe we've run into issues with these upgrading and breaking stuff, is there a strong case for unpinning?

I haven't run into any issues other than more linting errors. We were only pinned because we hit a bug with the latest version at some point. We should be pinned to something more recent.

agjohnson · 2017-05-24T21:35:43Z

This looks to raise even more linting problems, we can address this after #2819 and all the other linting PRs are merged.

This addresses no-else-return, which removes a `return` in an `else` after an `if` that already has a `return`. This makes the default return more obvious in most cases.

agjohnson · 2017-06-13T20:28:05Z

I've updated this with the new rules that a modern pylint brought in, namely:

child class method arguments differ from super class arguments
a few cases of variables being redefined
using if len(array) > 0 -> if array

I also broke out, in a separate commit 0ab73a3, removing unnecessary else statements after a conditional with a return. It might be easier to review as it's own commit.

For the differing arguments, i followed the guideline of:

Match the arguments if we were being lazy and didn't replicate the arguments
If there are a number of arguments and it doesn't make sense to replicate the call signature, mark as pylint disabled.
Mark as disabled if there is an actual case for it

agjohnson · 2017-06-13T20:33:26Z

readthedocs/doc_builder/base.py

@@ -48,7 +48,7 @@ def force(self, **__):
        log.info("Forcing a build")
        self._force = True

-    def build(self, id=None, **__):  # pylint: disable=redefined-builtin
+    def build(self):


As far as i could tell, there were no child classes using any arguments on this method, and no calls with arguments. It seems safe to remove this, unless there is something hidden using these.

ericholscher

Looks good, just a few small nits/questions

ericholscher · 2017-06-13T21:15:38Z

prospector-more.yml

+  - settings
+  - tastyapi
+  - templates
+  - vcs_support


Rinse and repeat :)

ericholscher · 2017-06-13T21:15:55Z

readthedocs/bookmarks/views.py

+            json.dumps({'removed': True}),
+            status=200,
+            content_type="application/json"
+        )


Should we just delete this file?

Perhaps with another PR? I don't think the bookmarks code needs to live in our repo

ericholscher · 2017-06-13T21:16:56Z

readthedocs/builds/forms.py

-    def save(self, *args, **kwargs):
-        obj = super(VersionForm, self).save(*args, **kwargs)
+    def save(self, commit=True):
+        obj = super(VersionForm, self).save(commit=commit)
        if obj.active and not obj.built and not obj.uploaded:
            trigger_build(project=obj.project, version=obj)
        return obj


I do worry that this class of change will have spiraling effects that only show up in run time. Is there a reason we don't want to continue accepting args & kwargs for these things?

In this case, we should have never pushed *args, **kwargs, as it was just lazy. commit=True is more explicit and is now identical to the underlying method signature.

In cases where there are many arguments, I don't know what's best there. *args, **kwargs is flexible for future compatibility, so probably in that case it's fine to ignore the linting error and use the *args, **kwargs catch all.

ericholscher · 2017-06-13T21:21:53Z

readthedocs/projects/version_handling.py

@@ -36,7 +36,7 @@ def prune_major(self, num_latest):
        all_keys = sorted(set(self._state.keys()))
        major_keep = []
        for __ in range(num_latest):
-            if len(all_keys) > 0:
+            if all_keys:


Seems like the previous approach was more explicit (eg. it must be a list, and will raise an exception if None is there). Not sure if we care, but it does seem like this is changing logic some.

For this particular case, all_keys will always be a sequence, so this logic hasn't changed.

If there is a case where the sequence being tested could be None, we should test for that instead of relying on an exception being thrown from len(). if len(sequence) is also described as an anti-pattern in pep8, so I tend to agree with this sort of change.

ericholscher · 2017-06-14T22:13:46Z

@agjohnson believe this can be closed?

agjohnson · 2017-06-15T00:57:53Z

Yup, it should be all merged. In fact, not really sure why the merge to master didn't close this PR automatically.

agjohnson added the PR: work in progress Pull request is not ready for full review label May 22, 2017

agjohnson modified the milestone: Cleanup May 22, 2017

agjohnson requested a review from ericholscher May 24, 2017 21:08

agjohnson force-pushed the tox-depends branch from 22d5153 to 22a726b Compare May 24, 2017 21:26

ericholscher reviewed May 24, 2017

View reviewed changes

agjohnson added the Status: blocked Issue is blocked on another issue label May 24, 2017

agjohnson mentioned this pull request Jun 9, 2017

Alternate approach to enhancing Python 3 support #2918

Closed

agjohnson added 4 commits June 9, 2017 09:51

Try unpinning testing reqs

a581398

Bump up prospector higher level strictness

fcaf401

Linting issues: arguments-differ, len-as-condition, redefined variables

8474b02

Remove unnecessary elses

0ab73a3

This addresses no-else-return, which removes a `return` in an `else` after an `if` that already has a `return`. This makes the default return more obvious in most cases.

agjohnson force-pushed the tox-depends branch from 22a726b to 0ab73a3 Compare June 13, 2017 20:02

agjohnson commented Jun 13, 2017

View reviewed changes

ericholscher approved these changes Jun 13, 2017

View reviewed changes

agjohnson mentioned this pull request Jun 13, 2017

Finish linting on py3ish code #2949

Merged

agjohnson added PR: ready for review and removed PR: work in progress Pull request is not ready for full review Status: blocked Issue is blocked on another issue labels Jun 14, 2017

agjohnson closed this Jun 15, 2017

agjohnson removed the PR: ready for review label Jun 15, 2017

stsewd deleted the tox-depends branch September 6, 2018 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try unpinning testing reqs #2863

Try unpinning testing reqs #2863

agjohnson commented May 22, 2017

ericholscher May 24, 2017

agjohnson Jun 13, 2017

ericholscher May 24, 2017

agjohnson May 24, 2017

agjohnson commented May 24, 2017

agjohnson commented Jun 13, 2017 •

edited

Loading

agjohnson Jun 13, 2017

ericholscher left a comment

ericholscher Jun 13, 2017

ericholscher Jun 13, 2017

agjohnson Jun 14, 2017

ericholscher Jun 13, 2017

agjohnson Jun 14, 2017

ericholscher Jun 13, 2017

agjohnson Jun 14, 2017

ericholscher commented Jun 14, 2017

agjohnson commented Jun 15, 2017

Try unpinning testing reqs #2863

Try unpinning testing reqs #2863

Conversation

agjohnson commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson commented May 24, 2017

agjohnson commented Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Jun 14, 2017

agjohnson commented Jun 15, 2017

agjohnson commented Jun 13, 2017 •

edited

Loading