Skip to content

Conversation

@ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Jun 6, 2025

Closes #66

Comment on lines 13 to 16
# - repo: https://github.com/keewis/blackdoc
# rev: v0.3.9
# hooks:
# - id: blackdoc
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll enable that in another PR to avoid a lot of code churn.

- id: check-ast
- id: debug-statements
- id: end-of-file-fixer
# - id: check-docstring-first
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code base uses docstrings as a way to do multi-line comment. We need to change that to be able to build clear docs and to run this check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It passes Ruff as is -- though I think that's a bad practice, so it would be good to clean that up. Maybe now, as that would make for less churn later.

Ideally, major format changes happen all in one commit ...

But your call, either way is good with me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project uses a small subset of all the lints ruff can run. While we can enable "ruff all" at some point, this particular line is focused on better docs rendering. I'll address it later b/c it may require some group discussion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused -- we hve ruff config in the pyproject.yaml, currently:

[tool.ruff]
builtins = ["ellipsis"]
extend-exclude = ["xarray_subset_grid/_version.py"]
target-version = "py310"
# Use a longer line length.
line-length = 100

[tool.ruff.lint]
ignore = [
  "E402", # module level import not at top of file
  "E731", # do not assign a lambda expression, use a def
]
select = [
  "F",   # Pyflakes
  "E",   # Pycodestyle
  "W",
  "TID", # flake8-tidy-imports (absolute imports)
  "I",   # isort
  "UP",  # Pyupgrade
]
extend-safe-fixes = [
  "TID252", # absolute imports
]

So shouldn't we simply call ruff check --fix (or whatever the incantation is) and leave all the config in the pyproject file?

But in any case -- yes, as for right now, minimal churn, and we can decide to nail down / update the project style requirements in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what pre-commit does.

hooks:
- id: ruff
args: ["--fix", "--show-fixes"]
# - id: ruff-format
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If folks want to adopt ruff format in lieu of black, we should do that ASAP to avoid a lot of churn in the code base as the code grows in size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below -- yes, let's use ruff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruff format is one of the ruff lint's that is not used here. It is recommended as a black substitute. None of the selected ruff lints in the project.toml implements ruff format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it -- I added some ruff format config, and ran it, in the reformat branch.

It seems to have done a fine job, with not a lot of disruption.

Can you merge that in to this pre-commit branch and see how it goes?

Comment on lines 41 to 44
# - repo: https://github.com/woodruffw/zizmor-pre-commit
# rev: v1.6.0
# hooks:
# - id: zizmor
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll enable this one after #65 b/c that PR already fixes most if it.

@ChrisBarker-NOAA
Copy link
Contributor

Yes, let's stick with ruff -- that's what we've been using, and it currently passes a ruff check (as configured in the pyproject.yaml).

@ocefpaf
Copy link
Member Author

ocefpaf commented Jun 10, 2025

@ChrisBarker-NOAA the sdist is way too big and, before we publish this on PyPI, let me work on it a little bit to remove all test data from the sdist. Otherwise we risk reaching storage quota with just a few releases.

41M Jun  6 15:47 xarray_subset_grid-0.0.2.dev6+ga442a7d.d20250606.tar.gz

@ocefpaf ocefpaf marked this pull request as ready for review June 10, 2025 12:18
@ocefpaf ocefpaf requested a review from ChrisBarker-NOAA June 10, 2025 12:20
@ocefpaf
Copy link
Member Author

ocefpaf commented Jun 11, 2025

@ChrisBarker-NOAA this one is ready to go. I'll work on the data and test organization after it.

@ChrisBarker-NOAA
Copy link
Contributor

Thanks!

@ChrisBarker-NOAA
Copy link
Contributor

One question -- blackdoc formats code in docstrings, yes?

Doesn't ruff have that as well (at least as an option?) -- though as long as they do the same thing, no harm no foul.

@ChrisBarker-NOAA ChrisBarker-NOAA merged commit 9c6cf48 into ioos:main Jun 11, 2025
14 checks passed
@ocefpaf ocefpaf deleted the add_pre-commit-config branch June 11, 2025 16:36
@ocefpaf
Copy link
Member Author

ocefpaf commented Jun 11, 2025

One question -- blackdoc formats code in docstrings, yes?

Yes.

Doesn't ruff have that as well (at least as an option?) -- though as long as they do the same thing, no harm no foul.

One needs:

[tool.ruff.format]
docstring-code-format = true

In the pyproject.toml. I never used it in the past b/c it is kind of a new feature. If you want to use it we can add that line and remove blackdoc.

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.

Consider moving linting to pre-commits

2 participants