More ruff & pre-commit rules

[mfisher87]

No description provided.

[github-actions]

👈 Launch a binder notebook on this branch for commit 2bd77fd

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit b479d90

👈 Launch a binder notebook on this branch for commit b86a68a

👈 Launch a binder notebook on this branch for commit fa768ed

👈 Launch a binder notebook on this branch for commit 5011535

👈 Launch a binder notebook on this branch for commit facfd46

👈 Launch a binder notebook on this branch for commit 3956ca7

👈 Launch a binder notebook on this branch for commit a4cf9df

👈 Launch a binder notebook on this branch for commit 0b7447f

👈 Launch a binder notebook on this branch for commit 3c22771

[mfisher87]

Since pre-commit on the default branch is configured to use Black without isort, pre-commit.ci is fighting with the new formatter config. Because of that, we'll need to format again after merging. I prefer to turn off this auto-commit behavior so it can be controlled by the user posting an autofix comment.

[weiji14]

Fixed the crazy merge conflict. Maybe we should sort the rules under [tool.ruff.lint] alphabetically too?

[weiji14]

Won't be needed if #552 goes ahead.

[mfisher87]

💯

[mfisher87]

Fixed the crazy merge conflict

Thanks!

Interesting. I've not worked on a project that requires squash merges for more than a PR or two. It really causes problems for chained PRs. Is there a common practice for dealing with this? If we always updated the feature branch via rebase, then it would be a bit less messy.

Briefly, I'd like to ask if there's any way you can think of we can utilize your review time more efficiently! I really appreciate all the work you've been doing on this! Would you be OK with "conditional approvals" in cases like this one where there are some "must-change" items, like sort order or config keys, and some "prefer-change" items like removing the pyproject checker rule, and once the "must-change" items are addressed, the PR can merge without additional review? I'm totally OK with waiting for a final check, I'm just worried I'm taking a lot of your time! ❤️

[mfisher87]

I'm not sure what's up with Travis. I get a blank response: https://app.travis-ci.com/github/icesat2py/icepyx/builds/271864435

[weiji14]

Interesting. I've not worked on a project that requires squash merges for more than a PR or two. It really causes problems for chained PRs. Is there a common practice for dealing with this?

Best practice is to have small commits that don't change so many files at once 🙂 I've got a good side-by-side merge conflict resolving tool with Gitkraken, so it wasn't too bad to resolve things.

If we always updated the feature branch via rebase, then it would be a bit less messy.

Personally I dislike rebases because as a reviewer, it undos all the 'Viewed' checkboxes I've clicked on the 'Files Changed' tab, and I have to go back and re-read the lines of code (that may not have changed), instead of focusing on the lines that have actually changed. Also, if there are co-authored commits, some of those commits will become 'Unverified' because you do not have the co-author's GPG signing keys.

Briefly, I'd like to ask if there's any way you can think of we can utilize your review time more efficiently! I really appreciate all the work you've been doing on this! Would you be OK with "conditional approvals" in cases like this one where there are some "must-change" items, like sort order or config keys, and some "prefer-change" items like removing the pyproject checker rule, and once the "must-change" items are addressed, the PR can merge without additional review? I'm totally OK with waiting for a final check, I'm just worried I'm taking a lot of your time! ❤️

First off, I think you're doing a good job at this. Icepyx's development has been focused more on features for the longest time (i.e. as long as it works, doesn't matter if it's not formatted nicely). Usually in unformatted codebases like this, I'd use an incremental linter like darker to only ensure the lines I change are formatted nicely rather than doing blanket changes on the whole codebase. But I do think that it's time for icepyx to look a bit more professional, and having newer linters like ruff with quick autofix definitely helps, so really appreciate you taking the effort here with >2 PRs!

On the topic of "must-" vs "prefer-" changes, one way I usually do this is to pre-approve once all the "must-" changes are satisfied, and the "prefer-" changes is left to the contributor to decide if they want to apply it or not. Generally, I try to suggest changes that will make it easier for the next person looking at the code, whether it's by making it clear where a new Ruff rule will go in (hence alphabetical sorting), or minimizing the diff between commits (easier to do git blame later). If you don't mind waiting, it's usually good to leave PRs that are almost ready sitting there for a few days, just so there's a chance for more eyeballs to look at it. The process here is still quicker than traditional peer review 😆

[weiji14]

With that said ☝️, approving with one comment 🚀

[mfisher87]

Best practice is to have small commits that don't change so many files at once 🙂

That's my usual practice, that's the tradeoff with autofixes :) Please let me know if see any instances where I can break things up better!

Personally I dislike rebases because as a reviewer, it undos all the 'Viewed' checkboxes I've clicked on the 'Files Changed' tab, and I have to go back and re-read the lines of code (that may not have changed), instead of focusing on the lines that have actually changed

Agree, that's frustrating. On the flip-side, all of the commits from a dependent PR are in here :(

...on reviewing stuff...

That all sounds great! Do you have any thoughts about a process for reviewing v2? Do you want to review along the way before each branch is merged to v2, review along the way as time allows pre- or post-merge, review v2 before it's merged in to development, something else, or some combination? The v2 changes are going to be much more overhaul-y so I want to make sure I'm not spamming or disrupting anyone :)

[mfisher87]

Another thought on squash merges: on other projects I've used .git-blame-ignore-revs to ignore changes which are strictly formatting when using git blame (also works in GitHub's blame feature). Because we squash, we can't take advantage of that!

[codecov]

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.36%. Comparing base (2790edd) to head (3c22771).
Report is 27 commits behind head on development.

Files with missing lines	Patch %	Lines
icepyx/core/read.py	0.00%	2 Missing ⚠️
icepyx/core/query.py	50.00%	1 Missing ⚠️
icepyx/tests/test_auth.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #546      +/-   ##
===============================================
- Coverage        65.37%   65.36%   -0.02%     
===============================================
  Files               36       36              
  Lines             3038     3037       -1     
  Branches           531      531              
===============================================
- Hits              1986     1985       -1     
  Misses             970      970              
  Partials            82       82              

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