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

Fix Travis lint issue #3551

Merged
merged 9 commits into from Jan 26, 2018

Conversation

Projects
None yet
3 participants
@humitos
Member

humitos commented Jan 26, 2018

The new pylint has a couple of new features that makes our linting to fail. So, I fixed them as I think it's correct.

@@ -44,7 +44,7 @@ def handle(self, *args, **options):
version = Version.objects.get(id=build['version'])
latest_build = version.builds.latest('date')
if latest_build.date > max_date:
log.warn('{0} is newer than {1}'.format(
log.warning('{0} is newer than {1}'.format(

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

log.warn is deprecated

@@ -25,6 +25,7 @@ pylint:
max-line-length: 100
disable:
- logging-format-interpolation
- inconsistent-return-statements

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

I don't like this but you may don't agree.

def run_time(self):
        """Total command runtime in seconds."""
        if self.start_time is not None and self.end_time is not None:
            diff = self.end_time - self.start_time
            return diff.seconds

in that example, we are forced to put a return None as the last line of the method.

This comment has been minimized.

@davidfischer

davidfischer Jan 26, 2018

Contributor

In general, I think we should enforce consistent returns but it doesn't strictly have to be fixed in this PR. I removed this and ran it to see how many places we do this. There's around 35 functions with inconsistent returns.

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

Yes, there aren't too many functions. I added disabled it globally because I think it could be different opinions on this, so we need to agree if we want those explicit return or not.

If we are going in that direction I can fix those 35 functions.

@davidfischer

Overall I think this is an improvement. We could more strictly lint but that is an enhancement over this and I think this is fine to get the tests back into a working state.

@@ -20,7 +20,7 @@ class Meta(object):
'largest',
)
def __init__(self, instance=None, *args, **kwargs):
def __init__(self, instance=None, *args, **kwargs): # noqa

This comment has been minimized.

@davidfischer

davidfischer Jan 26, 2018

Contributor

I'm wondering if we should add keyword-arg-before-vararg to prospector.yml. Perhaps it's better to just add a few noqas and not introduce the bug any further (as you did).

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

I like prospector informing about this, but there are some particular cases (like this one) that we want to make an exception. I decided to add noqa instead of pylint: disable= because the line gets to long and then pep8 start failing, so I need to also disable that one and it ends ugly.

@@ -25,6 +25,7 @@ pylint:
max-line-length: 100
disable:
- logging-format-interpolation
- inconsistent-return-statements

This comment has been minimized.

@davidfischer

davidfischer Jan 26, 2018

Contributor

In general, I think we should enforce consistent returns but it doesn't strictly have to be fixed in this PR. I removed this and ran it to see how many places we do this. There's around 35 functions with inconsistent returns.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jan 26, 2018

Looks good to fix our build. Can address the other lingering issues later.

@ericholscher ericholscher merged commit 7be1b80 into master Jan 26, 2018

1 check passed

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

@stsewd stsewd deleted the humitos/lint/fix branch Aug 15, 2018

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