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

Fix Travis lint issue #3551

Merged
merged 9 commits into from Jan 26, 2018
Merged

Fix Travis lint issue #3551

merged 9 commits into from Jan 26, 2018

Conversation

@humitos
Copy link
Member

@humitos 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(
Copy link
Member Author

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

log.warn is deprecated

Loading

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

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor

@davidfischer davidfischer Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

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.

Loading

@@ -20,7 +20,7 @@ class Meta(object):
'largest',
)

def __init__(self, instance=None, *args, **kwargs):
def __init__(self, instance=None, *args, **kwargs): # noqa
Copy link
Contributor

@davidfischer davidfischer Jan 26, 2018

Choose a reason for hiding this comment

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

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).

Loading

Copy link
Member Author

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

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

@davidfischer davidfischer Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 26, 2018

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

Loading

@ericholscher ericholscher merged commit 7be1b80 into master Jan 26, 2018
1 check passed
Loading
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants