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 issues reported by pyflakes and add pyflakes step to ci, fixes #3756 #3760
Conversation
840a39e
to
ce153d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3760 +/- ##
=======================================
Coverage 93.77% 93.78%
=======================================
Files 162 162
Lines 19576 19595 +19
=======================================
+ Hits 18358 18377 +19
Misses 1218 1218
Continue to review full report at Codecov.
|
Thanks @maxnoe! I think the mostly looks fine, except the places where the
I have a slight preference for the old scheme with a combination of |
That is only true for python import semantics. It is not true for sphinx automodapi, I think. That one requires I would have to check if the autogenerated docs changed... |
@maxnoe It seems there is a |
It seems there is no way of letting pyflakes (or pylint) resolve the star imports. Which is a bit sad... |
Thanks @maxnoe I think in general this PR is needed, and in particular adding @adonath I would go with this PR, but it still needs some modifs.
|
other differences found in the API docs produced with this PR:
|
This is probably because they are defined in a higher level module and sphinx is not able to figure that out. That's quite unusual. They are correctly linked above in I think the best solution would be to just remove these two classes from the lower level namespace |
Is this really something that should be documented? It's a module level constant in a cython extension.
fixed |
Thanks @Bultako for checking! So we just probably keep was @maxnoe is proposing here. It's non-ideal because it increases the effort of bookkeeping (e.g. if new classes and methods are added, they have to be added in 2 places in addition to the actual implementation), but complete documentation is more important. |
@adonath can we precise then this in the Developer documentation? Because it is not so obvious... |
Thanks @maxnoe
From my side if you just rebase we could merge this PR, I could open a new one to fix ordering and the links to Parameters classes. |
0928239
to
67e13f9
Compare
Done
I'd suggest to remove both classes from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @maxnoe
I've written some comments in between the lines of code to finish with the alphabetical sorting. As for your proposed solution to link to the Parameter and Parameters docstrings, you can do it in this PR or in another different one. It's up to you.
@Bultako Went over it again, hope it's ok now |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@maxnoe |
@Bultako I went over all If that doesn't match what you proposed, you did some manual ordering? |
@maxnoe |
Manually according to which rules? You asked for alphabetic sorting. Yet you diverged from it apparently. So we need to define a sorting criteration which developers can follow, if we don't want that to be alphabetic. |
Ok, sorry, it seems I missed some files due to the limit on how many tabs neovim can open at the same time.... Now everything containing an For future reference, this does this automatically:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @maxnoe
It seems it worked now :)
If you can replace the single quotes by double quotes in the __all__
lists, we merge it 👍🏻
Done |
Thanks @maxnoe ! |
Description
This fixes all issues reported by pyflakes in the gammapy package. Most of them were related to star imports and missing
__all__
statements but some also were genuine issues, unused imports, f-strings without placeholders, unused variables and the like.