Conversation
efiring
left a comment
There was a problem hiding this comment.
Minor questions; overall, I like the cleanups.
gsw/geostrophy.py
Outdated
| if lon.shape != lat.shape or lon.ndim != 1: | ||
| raise ValueError('lon, lat must be 1-D and matching; found shapes' | ||
| ' %s and %s' % (lon.shape, lat.shape)) | ||
| ' {} and {}'.format(lon.shape, lat.shape)) |
There was a problem hiding this comment.
Might as well go all the way to an f-string.
There was a problem hiding this comment.
Will do. I'm not sure why pyupgrade, even though it is set to py38, stopped at .format.
gsw/geostrophy.py
Outdated
| ' {} and {}'.format(lon.shape, lat.shape)) | ||
|
|
||
| if geo_strf.ndim not in (1, 2): | ||
| raise ValueError('geo_strf must be 1-D or 2-d; found shape %s' |
There was a problem hiding this comment.
I wonder why the old percent syntax is changed above, but not here?
There's actually nothing wrong with the old syntax; it isn't going to go
away. But in most cases, f-strings are more readable. I would certainly
use them for new code.
There was a problem hiding this comment.
This comment somehow got connected to the wrong line. A few lines below here there is an unchanged line that has the percent syntax, and presumably ruff missed it.
There was a problem hiding this comment.
Probably a bug in pyupgrade via ruff. I'll investigate and report upstream.
There was a problem hiding this comment.
Looks like we are in the situation in the note here: https://github.com/asottile/pyupgrade#f-strings
pyproject.toml
Outdated
| ] | ||
|
|
||
| "gsw/_utilities.py" = [ | ||
| "B904", |
There was a problem hiding this comment.
For each of these skips, please add a short comment identifying what the rule addresses.
| sphinx | ||
| sphinx_rtd_theme | ||
| twine | ||
| xarray |
There was a problem hiding this comment.
@DocOtak xarray is not compatible with pandas>=2 yet. I don't think we miss a lot b/c the tests are already on an import_skip and I believe you do have better testing in gsw_xarray upstream, right?
There was a problem hiding this comment.
We test the full (apparent) API surface of GSW-Python in in gsw_xarray, so I wouldn't be too worried about breakages.
Merge only after #126.This one is ready for review!