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

feat(requirements): Support flake8 v5 #46

Merged
merged 2 commits into from Aug 9, 2022
Merged

feat(requirements): Support flake8 v5 #46

merged 2 commits into from Aug 9, 2022

Conversation

DmytroLitvinov
Copy link
Contributor

@DmytroLitvinov DmytroLitvinov commented Aug 2, 2022

Closes #45

@jonyscathe
Copy link

I am guessing that this is what is stuffing up those tests: PyCQA/flake8#1609

Not sure the best way around that... I don't think it causes any issues, but does mean a few tests aren't quite valid anymore.

Also, given how infrequently this project is updated, perhaps the flake8 upper bound should just be completely removed?

@mschwager
Copy link
Contributor

Yes, those TestFlake8Extension tests rely a bit on the implementation details of flake8. Not ideal, but it was the only way I could test some of the non-standard optimizations done in dlint.

The changelog states (https://flake8.pycqa.org/en/latest/release-notes/5.0.0.html):

Properly differentiate between explicitly ignored / selected and default ignored / selected options (See also #284, #1576, #1609).

Is this just a matter of updating the defaults in create_options in tests/test_extension.py?

@jonyscathe
Copy link

@mschwager changing the defaults in create_options can almost do it.

Currently the ignore and extend_ignore tests fail because all linter_classes come up as ignored given that they are not explicitly selected (only implicitly selected with the extended_default_select default of ["DUO"]).
Those two tests can pass if the default value for the "select" kwarg is changed to ["DUO"].
And if that change is made then the only failed test is test_flake8_extension_get_linter_classes_extended_default_select

And I'm not sure that the extended_default_select option can be tested using the current test methods. The extended_default_select setting doesn't appear to have any affect on the engine.decision_for() value for any given code. If a code (or "DUO" for all DUOXXX codes) is explicitly selected with "select" or "extend_select" and its code is not in the ignore or extend_ignore list then its decision_for() value is style_guide.Decision.Selected. If it isn't explicitly selected with "select" or "extend_select" then its decision_for() value will be style_guide.Decision.Ignored no matter what the value of extended_default_select is.

I'm not entirely sure how important the extended_default_select test is for dlint. Nor how important the behaviour of get_linter_classes() is with different option args so I'm not sure what the best way forward is for that test.

@mschwager
Copy link
Contributor

Thanks for digging into this.

Now I remember where I got the initial create_options code: https://github.com/PyCQA/flake8/blob/main/tests/unit/test_decision_engine.py

Copying the latest create_options changes from that file to Dlint made the tests pass for me. In other words, select, extend_select, ignore, and extend_ignore must default to None, not []. If you add those changes to this PR I suspect CI will succeed and we'll be good to go 🎉

That change shouldn't affect any functionality in Dlint. Since None vs. [] is an implementation detail of CLI arguments passed to flake8, the Dlint optimizations in get_linter_classes shouldn't be affected, and it will do the correct thing in flake8<5.0.0 and flake8>=5.0.0.

@jonyscathe
Copy link

Ah yep, awesome. Sounds good to me.
@DmytroLitvinov are you able to make those changes from https://github.com/PyCQA/flake8/blob/main/tests/unit/test_decision_engine.py into tests/test_extension.py since this is a PR from your fork? Or if not I guess I can make a PR into your fork (or possibly just a new PR might be simpler given this has minimal changes)

@DmytroLitvinov
Copy link
Contributor Author

Good morning everyone 👋
I have pushed changes. Could some of the maintainers run CI?

@mschwager
Copy link
Contributor

It passed! Thanks again for the PR, I will cut a new release of Dlint now 👍

@mschwager mschwager merged commit a742b4f into dlint-py:master Aug 9, 2022
@mschwager
Copy link
Contributor

It's up: https://pypi.org/project/dlint/0.13.0/

@DmytroLitvinov DmytroLitvinov deleted the patch-1 branch August 10, 2022 05:15
@DmytroLitvinov DmytroLitvinov restored the patch-1 branch August 10, 2022 05:15
@DmytroLitvinov DmytroLitvinov deleted the patch-1 branch August 10, 2022 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for flake8==5.x.x
3 participants