Autofix flake8 ignores E711, E712, E714, F401, F841 with Ruff

[mfisher87]

I haven't added Ruff as a dependency, but I do think we should replace flake8 with it because it's capable of autofixing. I've seen an immense difference in cognitive load and labor over the last year or so I've used Ruff! Thoughts?

[github-actions]

👈 Launch a binder notebook on this branch for commit 65e7ee4

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit ff9774e

👈 Launch a binder notebook on this branch for commit 8b3f79f

👈 Launch a binder notebook on this branch for commit ad95e2e

👈 Launch a binder notebook on this branch for commit 4916eb2

[codecov]

Codecov Report

Attention: Patch coverage is 58.62069% with 24 lines in your changes missing coverage. Please review.

Project coverage is 66.05%. Comparing base (91ca3c9) to head (4916eb2).
Report is 47 commits behind head on development.

Files with missing lines	Patch %	Lines
icepyx/core/variables.py	0.00%	17 Missing ⚠️
icepyx/core/spatial.py	40.00%	0 Missing and 3 partials ⚠️
icepyx/quest/dataset_scripts/argo.py	71.42%	0 Missing and 2 partials ⚠️
icepyx/core/query.py	0.00%	1 Missing ⚠️
icepyx/core/visualization.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #542      +/-   ##
===============================================
- Coverage        66.27%   66.05%   -0.23%     
===============================================
  Files               36       36              
  Lines             3072     3049      -23     
  Branches           537      537              
===============================================
- Hits              2036     2014      -22     
+ Misses             948      947       -1     
  Partials            88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[weiji14]

I haven't added Ruff as a dependency, but I do think we should replace flake8 with it because it's capable of autofixing. I've seen an immense difference in cognitive load and labor over the last year or so I've used Ruff! Thoughts?

Maybe we can do a two-step process. Fix (most of) the flake8 errors here, and then another PR to migrate from flake8 -> ruff? To make sure the transition is ok, you can leave some small flake8 errors around so we can check that the autofix works later.

[mfisher87]

@weiji14 #543 I was already working on it! 🚀

[weiji14]

Cool, I'll just note that code coverage on icepyx is a bit low (<70%), so you'll want to be a bit careful with refactoring 🙂

[mfisher87]

Good call - as I'm working on #544 I hope to increase coverage!

[JessicaS11]

I could have sworn I'd handled most of the "imported but unused" errors (F401). Apparently not... this looks great, @mfisher87!

[weiji14]

Pre-approving since this looks good (mostly changes of == -> is and removing unused imports). Should be ok to merge once #542 (comment) is addressed.

[mfisher87]

There's a failing test! I found that test is non-deterministic. It compares two lists, but relies on the order of one of those lists not being changed when it's converted to a set and then back to a list. The reason this is a problem is that sets are unordered, but lists are ordered:

Being an unordered collection, sets do not record element position or order of insertion.

We can verify the effect. Takes a few tries.

$ python -c "print(list({'temperature', 'salinity'}))"
['temperature', 'salinity']

$ python -c "print(list({'temperature', 'salinity'}))"
['temperature', 'salinity']

$ python -c "print(list({'temperature', 'salinity'}))"
['temperature', 'salinity']

$ python -c "print(list({'temperature', 'salinity'}))"
['temperature', 'salinity']

$ python -c "print(list({'temperature', 'salinity'}))"
['temperature', 'salinity']

$ python -c "print(list({'temperature', 'salinity'}))"
['salinity', 'temperature']

I believe, under the hood, the set is sorted by object hash:

$ python -c "print(hash('temperature'))"
-7033926737989545164

$ python -c "print(hash('temperature'))"
7247647540643673123

The test:

icepyx/icepyx/tests/test_quest_argo.py

Lines 60 to 63 in 91ca3c9

	reg_a.params = ["temperature", "salinity"]
	exp = list(set(["temperature", "salinity"]))
	assert reg_a.params == exp

4916eb2 fixes. Not clear if this test is actually useful, but I left that exercise for another time :)

[JessicaS11]

There's a failing test! I found that test is non-deterministic.

This test has been ailing us for some time (and often if I just rerun the CI it passes). I think the list(set()) bit got added to try and fix that (it clearly didn't!).

Not clear if this test is actually useful

If memory serves, there's a setter for that method so it might be trying to get at defaults/valid inputs. If that's the case, I'd be happy to learn if there's a better way!

[mfisher87]

For this test, should order matter? We can do a set comparison instead of list comparison if it doesn't.

[mfisher87]

https://app.travis-ci.com/github/icesat2py/icepyx/builds/271849698

I think this answers my question: The setter can change the order of the input data. I will change the test to do set comparison!

PR: #550