search

bjodah / chempy

⚗ A package useful for chemistry written in Python
BSD 2-Clause "Simplified" License
548 stars 79 forks source link

Convert from pytest-pep8 to pytest-flake8 #187

Closed jeremyagray closed 3 years ago

jeremyagray commented 3 years ago

This converts from using pytest-pep8 to pytest-flake8 and adds a --lint-only option for linting without running other tests.

bjodah commented 3 years ago

Hi @jeremyagray, on one of my systems I got the latest version of pytest and my old markers for "slow" and "veryslow" seem to have stopped working. Right now, I don't quite have the time to look into what has changed in pytest's API, so I'm leaning toward dropping those markers for now (see gh-188).

If we merge gh-188, I suspect you will need to merge master back into this branch (and possibly deal with some merge conflicts in e.g. conftest.py). Let me know what you think.

jeremyagray commented 3 years ago

I'll pull it and have a look, but I don't think rebasing it will be too big an issue. I do have a significant portion of the linting problems corrected and was just waiting on this PR. As for the markers, I was running all the tests anyway and I have no idea if anything changed, but I will look as I'm rebasing and see if I can determine anything.

Also, I've noticed in my other projects that the pytest-flake8 checks seem to be a superset of pytest-flakes, making the flakes checks redundant. I'm not sure if it's worth doing flakes if flake8 is already doing those checks (and more).

I noticed you updated to sundials-5.5.0 too. Did that affect the multiple includes problems with pycvodes or could you tell?

On Mon, Nov 23, 2020 at 3:58 PM Bjorn notifications@github.com wrote:

Hi @jeremyagray https://github.com/jeremyagray, on one of my systems I got the latest version of pytest and my old markers for "slow" and "veryslow" seem to have stopped working. Right now, I don't quite have the time to look into what has changed in pytest's API, so I'm leaning toward dropping those markers for now (see gh-188 https://github.com/bjodah/chempy/pull/188).

If we merge gh-188 https://github.com/bjodah/chempy/pull/188, I suspect you will need to merge master back into this branch (and possibly deal with some merge conflicts in e.g. conftest.py). Let me know what you think.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/pull/187#issuecomment-732450128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS3EBHT4Y5GSTK4TFULSRLLJLANCNFSM4TGCXZKA .

-- Jeremy A. Gray

bjodah commented 3 years ago

Sorry, I did not realize you were waiting for this PR to be merged. Feel free to ping me whenever you feel it's time for me to take a closer look.

I'm fine with dropping flakes if we use flake8 (and yes, it do sound like it aims to be a superset).

Regarding sundials-5.5.0. I might solve some issues, however, I have never been able to reproduce those conflicting declaration errors om my machine, so I cannot tell for certain.

jeremyagray commented 3 years ago

I've got the original one rebased and working, but I'll try dropping the pytest-flakes tests and make sure nothing gets lost. pytest-flakes is still a dependency of pytest-flake8, but I think the flakes tests can be safely dropped. I need to double check that all the warnings that used to be ignored are still ignored with flake8.

I did some looking in v0.7.12 about the slow/veryslow tests and it appears pytest has been doing all the tests for a while. The test marker part is still the same, but test selection appears to have changed at some point in the past. I could run all the tests with python -m pytest -ra or just the slow ones with python -m pytest -ra -m 'slow', etc., but I could not get any change with the --slow/--veryslow options in conftest.py. But since running all the tests with linting only takes ~30 s on my (older) notebook, I think dropping the distinction is a good idea. Only the very slow tests in kinetics are slow enough for me to notice.

On Tue, Nov 24, 2020 at 3:12 AM Bjorn notifications@github.com wrote:

Sorry, I did not realize you were waiting for this PR to be merged. Feel free to ping me whenever you feel it's time for me to take a closer look.

I'm fine with dropping flakes if we use flake8 (and yes, it do sound like it aims to be a superset).

Regarding sundials-5.5.0. I might solve some issues, however, I have never been able to reproduce those conflicting declaration errors om my machine, so I cannot tell for certain.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/pull/187#issuecomment-732762249, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS3A3KSN7EPGZDL54UDSRN2IZANCNFSM4TGCXZKA .

-- Jeremy A. Gray

bjodah commented 3 years ago

Great, thank you for looking into this with such detail. Dropping pytest-flakes sounds good to me.

jeremyagray commented 3 years ago

It was complete overlap between pytest-flake8 and pytest-flakes, so I removed the pytest-pep8 and pytest-flakes stuff and integrated everything into pytest-flake8. So the PR (#187) should be ready for v0.8.1.

79 of 118 files are currently not passing the linting check and CI is flagging it, but I've got a lot of those fixed and I can start PRs for those as well.

On Tue, Nov 24, 2020 at 7:57 AM Bjorn notifications@github.com wrote:

Great, thank you for looking into this with such detail. Dropping pytest-flakes sounds good to me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/pull/187#issuecomment-732988652, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS254A4L666RMX7DHKTSRO3TLANCNFSM4TGCXZKA .

-- Jeremy A. Gray

bjodah commented 3 years ago

OK sounds good. But let's disable the style checks in the CI setup for temporarily then, and then maybe sequentially re-enable them in the PRs which fixes the style issues.

I'd like to keep CI passing on master branch, since otherwise one might miss actual regressions in unrelated pull requests. How does that sound to you?

jeremyagray commented 3 years ago

I agree; I wouldn't break CI. But there were fewer real linting issues than I thought. Most were whitespace around operators or breaking around operators and if those are disabled, there are only about 15 files that need fixes. I have those fixed (all 447 tests passing) and should be able to merge those fixes into the current PR (#187) branch if you want to get everything done in one PR.

On Thu, Nov 26, 2020 at 4:50 AM Bjorn notifications@github.com wrote:

OK sounds good. But let's disable the style checks in the CI setup for temporarily then, and then maybe sequentially re-enable them in the PRs which fixes the style issues.

I'd like to keep CI passing on master branch, since otherwise one might miss actual regressions in unrelated pull requests. How does that sound to you?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/pull/187#issuecomment-734225449, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS5QKDS4BY47JOJMRDDSRYXGNANCNFSM4TGCXZKA .

-- Jeremy A. Gray

bjodah commented 3 years ago

Sounds promising! Sure, let's get all done in this PR then.

jeremyagray commented 3 years ago

Finally got this finished.

As I wrote in the commit, a large number of a few types of warnings are being ignored but I believe an automated (black or autopep8) fix would be the best way to go if there is a desire to remove the remaining warnings. I like black and use it frequently, but it's not configurable so you have to be willing to let it handle the formatting for you.

bjodah commented 3 years ago

Nice job!

I've looked at black, but I disliked the complete lack of ability to configure it. Because I find that sometimes, especially in tests, when there are e.g. matrices with numbers, aligning them by varying number of spaces really help readability... But perhaps black has gotten "better" in this area?

I haven't heard of autopep8, can it be explicitly disabled for e.g. certain lines by appending # noqa or something similar?

I'm open to reconsider my stance on black, perhaps we could look at what the change would look like in a PR?

jeremyagray commented 3 years ago

I mentioned black only for completeness. I've tried it, I like it, and I've had no issues with it but I have my doubts about the readability of matrices and calculations afterwards. For general use, using black with a pre-commit hook solves any formatting issues that might arise. I've not tried autopep8, but I think it's based on flake8 and not as opinionated as black and I doubt it's much different from the checks that are currently in CI.

That said, I'll do a PR for black just out of curiosity.

On Mon, Dec 28, 2020 at 3:56 PM Bjorn notifications@github.com wrote:

Nice job!

I've looked at black, but I disliked the complete lack of ability to configure it. Because I find that sometimes, especially in tests, when there are e.g. matrices with numbers, aligning them by varying number of spaces really help readability... But perhaps black has gotten "better" in this area?

I haven't heard of autopep8, can it be explicitly disabled for e.g. certain lines by appending # noqa or something similar?

I'm open to reconsider my stance on black, perhaps we could look at what the change would look like in a PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/pull/187#issuecomment-751876461, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS6M4KIOIRQCV5N3I3LSXD5JDANCNFSM4TGCXZKA .

-- Jeremy A. Gray