Fix GH212, PP308

[tqa236]

Fix a few warnings by repo review: GH212, PP308

• xref Address repo-review suggestions #8239

[max-sixty]

This seems super verbose:

pytest -k move -ra
============================================================================ short test summary info =============================================================================
SKIPPED [1] xarray/tests/test_cupy.py:9: could not import 'cupy': No module named 'cupy'
SKIPPED [1] xarray/tests/test_sparse.py:20: could not import 'sparse': No module named 'sparse'
SKIPPED [1] xarray/tests/test_strategies.py:5: could not import 'hypothesis': No module named 'hypothesis'
SKIPPED [1] properties/test_encode_decode.py:9: could not import 'hypothesis': No module named 'hypothesis'
SKIPPED [1] properties/test_pandas_roundtrip.py:12: could not import 'hypothesis': No module named 'hypothesis'
================================================================= 3 passed, 5 skipped, 18117 deselected in 3.65s =================================================================

There are going to be hundreds of lines on a full test?!

[tqa236]

@max-sixty should I revert this change? As a side note, do you think it's worth adding a repo-review section in pyproject.toml to mark unfixed errors?

[max-sixty]

@max-sixty should I revert this change?

I would vote to not have this.

And would push back on this being recommended — is there someone who thinks this is a good idea in 90%+ of repos? Or what's the bar for having things as part of the standard? (not blaming you tbc!)

do you think it's worth adding a repo-review section in pyproject.toml to mark unfixed errors?

Would you mind providing more context?

[tqa236]

@max-sixty I removed this change as well.

About the second question, I think it would be nice if we can provide some context to future maintainers/contributors that we tried this rule and confirmed that it's not suitable for the project so that they don't spend time trying that again. Maybe as a form of comment, or a section in pyproject.toml that specifies which rules we want to enforce and which we want to ignore.

Relevant doc: https://github.com/scientific-python/repo-review?tab=readme-ov-file#configuration

[tool.repo-review]
select = ["A", "B", "C100"]
ignore = ["A100"]

[max-sixty]

Great idea!

[tqa236]

@max-sixty I added the config, please take a look.

The check no longer shows PP308 as failed: https://learn.scientific-python.org/development/guides/repo-review/?repo=tqa236%2Fxarray&branch=fix-repo-review-warning

[max-sixty]

👍

[max-sixty]

Great, thanks @tqa236

Setting to auto-merge. There's unrelated test failure from a dependency which should clear in a few hours (but will require re-running the test). Feel free to ping if it doesn't merge!