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

.flake8: max-complexity and ignore=C901 are mutually exclusive #54

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jul 14, 2020

NOTE: max-complexity should be 10, not 101! I have never seen a complexity level so high.

Also max-line-length and ignore=E501 are mutually exclusive. max-line-length should be 88, not 180.

NOTE: max-complexity should be 10, not 101!  I have never seen a complexity level so high.

Also max-line-length and ignore=E501 are mutually exclusive.  max-line-length should be 88, not 180.
@cclauss cclauss requested review from targos and ryzokuken July 14, 2020 08:29
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM overall.

max-complexity = 10
max-line-length = 88
extend-ignore = E203,C901,E501
max-complexity = 101
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: max-complexity should be 10, not 101! I have never seen a complexity level so high.

So now we need to refactor the code until this can be 10 again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A future work effort perhaps

max-line-length = 88
extend-ignore = E203,C901,E501
max-complexity = 101
max-line-length = 180
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be pretty easy to fix, right? Can't we use black to enforce 88?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but black will not autofix all long lines. For example, lines that are in triple quoted strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I remember @targos once mentioned that to me and I volunteered to fix it but didn't actually do it, so I guess that is my fault. I'll try to look into it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running black only reformats two files and only fixes 1 long line. This leaves 62 long lines that black will not fix.

These are nice tasks of first committers or interns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made issues #55 and #56 which can proceed after this PR lands.

Copy link

Choose a reason for hiding this comment

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

I've already made a PR fixing #56.

@ryzokuken ryzokuken merged commit 6a0c86f into master Jul 14, 2020
@ryzokuken ryzokuken deleted the flake8-set-max-complexity branch July 14, 2020 14:37
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.

None yet

3 participants