Added pylint config file

[ewu63]

Purpose

This PR adds a reasonable pylint config file for our codes. I have not added it as a CI job because I hesitate to enforce an additional check, but perhaps it's worthwhile. Given the verbosity of the config file, I have added a shell script which generates the config file, and in the future I anticipate any changes to the config file to be made via the shell script.

Again, comments are welcome on if/how we want to implement this.

[marcomangano]

Shall we turn this into a draft PR for now?

[ewu63]

I added a pylint job but made it so that it does not fail all jobs. Maybe this is a sufficient compromise to get this merged? It will probably still give the red X on all the repos (which will hopefully motivate people to fix stuff).

[A-CGray]

What does pylint add that we don't get from flake8?

[ewu63]

What does pylint add that we don't get from flake8?

A lot of stuff actually. Some example warnings/errors:

• Unused argument 'config' (unused-argument)
• Attribute 'centroid' defined outside __init__ (attribute-defined-outside-init)
• Redefining name 'points' from outer scope (line 101) (redefined-outer-name)
• Undefined variable 'offset' (undefined-variable)

We could probably set it to only raise Error/Warnings for now, and leave all the other stuff for later. Right now the output is pretty cluttered and it complains about some stuff that we don't really care.

[A-CGray]

I actually rescind my statement about the wildcard import warnings, it's annoying to get hundreds of warnings for a single wildcard import, but it's also stupid to call from numpy import * in the first place so perhaps that annoyance is justified.

[ewu63]

I have updated the pylint config:

• disabled all the (R)efactor, (I)nfo, and (C)onvention violations
• disabled unspecified-encoding and protected-access

This addresses most of the concerns above. However, there are a few violations that occur often in our code:

• attribute-defined-outside-init: this one I'm tempted to keep, because I strongly believe this is "incorrect" code. We inject class attributes all the time and it's very hard to track down things when debugging. It's also just not in the spirit of OOP.
• unused-variable/unused-argument: I'm willing to disable this one if it's too distracting, but it does foster good coding practice IMO, and this finds stuff that flake8 doesn't catch

Please try out this new config and let me know what you think.

[A-CGray]

I'm happy to keep unused-variable/unused-argument in there, at best unused arguments and variables are just easily fixed pieces of bad code, but quite often I would imagine this warning will find genuine bugs.

As for attribute-defined-outside-init, part of me understands why this is kinda bad practice, but another part of me thinks that if a particular attribute is being initialised in a particular method A then that is a clear indicator that method A has to be called before some other method B that relies on that attribute, and it's therefore a good thing that the user get an exception saying the attribute is not defined if they call B before A. If we initialised every attribute in __init__ then wouldn't we run the risk of the user being able to call B before A and producing undefined behaviour without triggering an exception? Either that or we would need to add a bunch of if statements checking that certain attributes have "proper" values rather than whatever dummy value they're initialiased within __init__? Not sure I'm making sense here, very open to being corrected.

I gave this new config a test run on my toy FEM repo and it highlighted some important things that I had no idea were issues. So for the most part I'm happy with this config as-is.

[ewu63]

I think relying on whether an attribute exists to define "correct" program behaviour is not a good way to go. It's much better to have error checks in place, and raise the correct error (with a meaningful message) if some function has not been called first. We already do this in some places by using hasattr, and I think that's just a worse way of doing things than if self.<something> is None, which is more explicit.

The really bad offender IMO is when one class injects attributes into another class instance, and then relies on the existence of that injected attribute for specific behaviour. That should be done via specific setter and getter functions, and this would get caught by these pylint checks.

[marcomangano]

I think relying on whether an attribute exists to define "correct" program behaviour is not a good way to go. It's much better to have error checks in place, and raise the correct error (with a meaningful message) if some function has not been called first. We already do this in some places by using hasattr, and I think that's just a worse way of doing things than if self.<something> is None, which is more explicit.

The really bad offender IMO is when one class injects attributes into another class instance, and then relies on the existence of that injected attribute for specific behaviour. That should be done via specific setter and getter functions, and this would get caught by these pylint checks.

I don't have so strong opinions but I am on board with this, setting everything in the __init__ as None is better than play hide-and-seek with attributes over the code.

[A-CGray]

I'm happy to merge this as-is then, we can always come back and make changes if the current settings prove to be more annoying than useful

[ewu63]

This PR does not yet add a CI job to run pylint. Do we want to do that here or in a separate PR?

[A-CGray]

This PR does not yet add a CI job to run pylint. Do we want to do that here or in a separate PR?

Yeah we should do that here

[ewu63]

This PR does not yet add a CI job to run pylint. Do we want to do that here or in a separate PR?

Yeah we should do that here

Apparently I was ahead of myself, it was added a few months ago in this PR. The job's enabled but not enforced. I may have to debug things a bit once it's merged in, since there's no way to test this as a PR.