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

LINT: disabled pylint flags should be re-enabled where possible, or disabled locally #196

Open
fkuehlein opened this issue Sep 25, 2023 · 7 comments · Fixed by #224, #233 or #236
Open
Labels
maintenance something should be improved or is outdated testing

Comments

@fkuehlein
Copy link
Collaborator

fkuehlein commented Sep 25, 2023

CI build previously failed due to pylint complaining about

E0110: Abstract class 'PlateCarree' with abstract methods instantiated (abstract-class-instantiated)

at two instances within climate.CartopyPlots.init() (see Travis CI log).

The flag was raised in the python=3.8-build only, later python version were fine. pyunicorn's handling of cartopy is perfectly in line with cartopy docs. Also — to my understanding — PlateCarree as not actually an abstract class itself, although some parent classes are (see cartopy source). What's more, there is a similar false-positive issue open at pylint.

Therefore, I will locally disable this flag for now. I am filing this issue, because this can only be a temporary solution and should be kept track of. Also, other currently disabled pylint-flags should be looked into, or disabled only locally where it makes sense.


On another side note: Deprecation/dropping of climate.MapPlots might be looked into as well. Is its functionality fully covered by climate.CartopyPlots and cartopy/matplotlib?

@fkuehlein fkuehlein changed the title LINT: pylint raising apparently false-positive abstract-class-instantiated when calling cartopy methods in climate.CartopyPlots LINT: disabled pylint flags should be re-enabled where possible, or disabled locally Sep 25, 2023
fkuehlein added a commit that referenced this issue Sep 25, 2023
- locally disable apparently false positive
  `E0110: abstract-class-instantiated` flag on calling
  `cartopy.crs.PlateCarree()`, which inhibited Travis CI builds to pass
- related issue: #196
@ntfrgl
Copy link
Member

ntfrgl commented Sep 25, 2023

I think that locally disabling this warning is the correct permanent strategy here, following Pylint's general advice as well as general practice. In particular, the underlying technical problem is arbitrarily complex (the task of statically predicting the dynamic presence or absence of methods, in a programming language with dynamically typed reflection, is in general equivalent to the halting problem). In this sense, Pylint should be understood as a collection of heuristics, and while improving the heuristics would be valuable, both the currently implemented heuristic and the only alternative heuristic suggested so far (in the 4 years since the issue you linked was opened) appear to be inadequate (yielding false positive) in our scenario for some interpreter versions.

Regarding other Pylint warnings that appear only in very few places, locating them would probably be beneficial as well, since it is trivial to find pylint: comments.


On another side note: Deprecation/dropping of climate.MapPlots might be looked into as well. Is its functionality fully covered by climate.CartopyPlots and cartopy/matplotlib?

This should be discussed with @jdonges and @zugnachpankow.

@fkuehlein
Copy link
Collaborator Author

Sounds totally reasonable now that you put it that way, thank you for the illustration.


This should be discussed with @jdonges and @zugnachpankow.

Also agreed, will put that forth on the next occasion ..

@zugnachpankow
Copy link
Contributor

zugnachpankow commented Sep 26, 2023

Also agreed, will put that forth on the next occasion ..

I am available to look into this with you. Basic features should be covered though.

@fkuehlein fkuehlein added the maintenance something should be improved or is outdated label Sep 26, 2023
@fkuehlein
Copy link
Collaborator Author

thanks for your availability @zugnachpankow, I will look into it myself and get back to you!

@fkuehlein
Copy link
Collaborator Author

Working on this cleanup on the linked branch. Some more severe issues found that I will address later:

  • 'MutualInfoClimateNetwork' has no '_path_lengths_cached' member (no-member) (multiple cases within class)
  • 'HavlinClimateNetwork' has no '_path_lengths_cached' member (no-member) (multiple cases within class)

@fkuehlein fkuehlein removed this from the Release 0.7 milestone Jan 25, 2024
@fkuehlein fkuehlein linked a pull request May 21, 2024 that will close this issue
@fkuehlein
Copy link
Collaborator Author

fkuehlein commented Jul 4, 2024

Most pylint flags have been resolved as of #224. Cleaned up Flake8 in #233.

Leave the issue open for the remaining pylint flags given in #224.

@fkuehlein fkuehlein reopened this Jul 4, 2024
@fkuehlein fkuehlein linked a pull request Jul 4, 2024 that will close this issue
@fkuehlein fkuehlein reopened this Jul 4, 2024
@fkuehlein fkuehlein linked a pull request Aug 13, 2024 that will close this issue
@fkuehlein
Copy link
Collaborator Author

fkuehlein commented Aug 13, 2024

"no-member" is fixed as of #236.

As stated in #224,

I would argue to keep

disable = ["duplicate-code", "invalid-name", "fixme", "missing-docstring", "no-else-return"]

for now.

Thus, "arguments-differ" and "no-name-in-module" are yet to be re-enabled and respective cases fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance something should be improved or is outdated testing
Projects
None yet
3 participants