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

TVB-2719 lint: Use same convention everywhere #63

Merged
merged 5 commits into from Aug 10, 2020

Conversation

ayan-b
Copy link
Member

@ayan-b ayan-b commented Aug 4, 2020

Closes #60

@ayan-b ayan-b self-assigned this Aug 4, 2020
@ayan-b
Copy link
Member Author

ayan-b commented Aug 4, 2020

I used black to format the code.

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #63 into trunk will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             trunk       #63   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           72        72           
=========================================
  Hits            72        72           
Impacted Files Coverage Δ
gdist.pyx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da6c8f5...0f400ad. Read the comment docs.

Copy link
Member

@liadomide liadomide left a comment

Choose a reason for hiding this comment

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

These changes look good for the Python part. How about the Cython part of this library? Is there a cython lint tool which we could apply?

@ayan-b
Copy link
Member Author

ayan-b commented Aug 5, 2020

Modified flake8 configuration to enable linting for cython files.

@ayan-b ayan-b changed the title lint: Use same convention everywhere TVB-2719 lint: Use same convention everywhere Aug 5, 2020
setup.cfg Outdated
[flake8]
filename = *.py, *.pyx
ignore = E501, W503
per_file_ignores = gdist.pyx: E999, E402, E266, E255, E227, E226, E225
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to copy with every ignored error code also the explanation for it (what we are ignoring).
e.g. E501 # Line too long (82 > 79 characters) ?

Copy link
Member

Choose a reason for hiding this comment

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

Is E402 | Module level import not at top of file ?
Usually I don't like the stile of an import in another place that the top, unless there is a good reason. Do we really need this exception here?

Copy link
Member

Choose a reason for hiding this comment

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

Also are these the ignores we really want:
E225 | Missing whitespace around operator E226 | Missing whitespace around arithmetic operator E227 | Missing whitespace around bitwise or shift operator
They would seem fitting to be obeyed, to me.

Copy link
Member Author

@ayan-b ayan-b Aug 7, 2020

Choose a reason for hiding this comment

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

Also are these the ignores we really want:
E225 | Missing whitespace around operator E226 | Missing whitespace around arithmetic operator E227 | Missing whitespace around bitwise or shift operator
They would seem fitting to be obeyed, to me.

For pyx files, flake8 will complain for the pointers (treated as multiplication operator in python) and address-of operator (bitwise and operator in python).

Is E402 | Module level import not at top of file ?
Usually I don't like the stile of an import in another place that the top, unless there is a good reason. Do we really need this exception here?

Flake8 treats the cimports as non-import code and thus complains about the subsequent imports as imports not being on the top.

Would it be possible to copy with every ignored error code also the explanation for it (what we are ignoring).
e.g. E501 # Line too long (82 > 79 characters) ?

Done.

@liadomide liadomide merged commit 59a2b99 into the-virtual-brain:trunk Aug 10, 2020
@ayan-b ayan-b deleted the lint branch August 10, 2020 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint: Use same convention everywhere
2 participants