mdolab / .github

0 stars 3 forks source link

Fix shared flake8 rules #47

Closed eirikurj closed 1 year ago

eirikurj commented 1 year ago

Purpose

Current flake8 rules are not applied as intended. The select statement only applies the patterns defined there, overwriting anything else, including the defaults. PR https://github.com/mdolab/.github/pull/45 removed E, F, W from the select, thus applying only A00, B patterns in our tests. To respect the defaults, but selectively enable values, we should instead use extend-select, to enable specific patterns.

There are two approaches possible:

  1. Use the defaults and supplement them with extend-select for any patterns we want to enable. Then use extend-ignore to ignore some unwanted enabled patterns.
  2. Continue to use select and manually specify every pattern we want to allow. Similarly, we need to use extend-ignore to ignore some unwanted enabled patterns.

This PR implements the former by removing any select statements, thus respecting all default patterns, but then selectively disabling certain patterns. The extend-select statement is not included in the current .flake8, as I am not aware of any special patterns we need to enable. The benefit of this approach is that we follow any defaults changed upstream, and we do not need to specifically enable plugin codes as we install them. They are automatically enabled as stated by the docs (see extend-select and select),

You usually do not need to specify this option as the default includes all installed plugin codes.

Open to discussions, comments, or concerns if there are any.

Expected time until merged

Soon as tests are not testing what we thought we are testing.

Type of change

Testing

Tested locally on several repositories. Please pull down and test locally as well.

Checklist

A-CGray commented 1 year ago

PR https://github.com/mdolab/.github/pull/45 removed E, F, W from the select, thus applying only A00, B patterns in our tests.

This is not true, the default list of errors flake8 checks includes a subset of the E, F, and W checks. PR #45 removed E, F and W from the select argument as this was enabling all E, F and W checks, including those that are not PEP compliant and in some cases contradict each other.

I agree with your conclusion, let's go with approach 1, maybe add in the extend-select line and comment it out so that it's obvious where to add things in future.

eirikurj commented 1 year ago

Added the extend-select commented out, as well as a comment.