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

Add some lint errors to .pylintrc disable list (#2123) #2142

Merged
merged 2 commits into from
Oct 27, 2019

Conversation

hramezani
Copy link
Contributor

Fixes #2123

Copy link
Collaborator

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I don't think any of these pylint errors make our code better. I'd prefer adding them to the ignore list at https://github.com/benoitc/gunicorn/blob/master/.pylintrc#L13

@hramezani hramezani changed the title Fix lint errors (#2123) Add some lint errors to .pylintrc disable list (#2123) Oct 24, 2019
@hramezani
Copy link
Contributor Author

@berkerpeksag I have added them to .pylintrc disable section.

@Djailla
Copy link

Djailla commented Oct 24, 2019

Except for import-outside-toplevel I find the warning pretty useful.

Especially for module, xxx = module, yyy which add no value here.

For the no else after break, it make the code more compact and avoid big if / else.

IMO only import-outside-toplevel should be in pylintrc

@hramezani
Copy link
Contributor Author

@Djailla I agree with you.
@berkerpeksag What do you think?

@hramezani
Copy link
Contributor Author

I added import-outside-toplevel to pylintrc disable list and fixed the rest of lint errors.

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

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

Thank you!

@benoitc
Copy link
Owner

benoitc commented Oct 26, 2019

I don't unrstand some changes in the reloader and gthread worker. how are they related? and what do they fix?

I mean the else in gthread is here for readability. this ˋif` may be enough but the logic is not explicit.

About the ˋcontinue` removal in the reloader module it follows the same logic and is more optimized (it continue the loop now and after evaluating the other sentences)

@hramezani
Copy link
Contributor Author

@benoitc I can revert the change gthread and reloader modules and add no-else-continue and no-else-break to pylintrc disable list.
It is okay for you?

@tilgovi
Copy link
Collaborator

tilgovi commented Oct 26, 2019

I'm neutral. I like using defaults for linters, rather than doing lots of customization. I also like some of these rules, some of the time.

For example, I really like when guards near the top of functions are under if, without an else:

def foo():
    if guard:
        return

    # no else because this is not about one thing _or_ the other, just about a precondition

It's rare that I find these lint rules to make the code worse, and if it does it can be locally disabled.

I'm 👍 for these changes the way they are in this PR right now.

@benoitc
Copy link
Owner

benoitc commented Oct 27, 2019

i'm agree with the general spirit. Although in this two cases, yes, I would prefer to keep it the current way. It's more readable. Rest of the patch is OK, thanks for this.

There is a bunch of refactoring that could be done later to modernize the current source but let's not all the warnings dictate all of it for now :)

@hramezani
Copy link
Contributor Author

pylint: disable=no-else-break added to workers/gthread.py
pylint: disable=no-else-continue added to reloader.py

@benoitc benoitc merged commit fa23cab into benoitc:master Oct 27, 2019
@benoitc
Copy link
Owner

benoitc commented Oct 27, 2019

thanks!

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.

lint CI error
5 participants