ENH: enable more ruff rules + make appropriate changes #415
Conversation
There was a problem hiding this comment.
Thanks for the cleanup @theroggy , just a few minor comments.
| datetime_kwargs = dict(format="ISO8601") | ||
| datetime_kwargs = {"format": "ISO8601"} |
There was a problem hiding this comment.
FWIW, I am not going to disagree merging this PR for this ;), but this is a change I dislike. When specifically creating keyword arguments, I like that dict(..) is closer in syntax to directly passing those to the function
There was a problem hiding this comment.
Yes, I remember the same remark being made in geopandas when this change was applied there :-):
geopandas/geopandas#2921 (comment)
There was a problem hiding this comment.
Ah, it seems I somewhat consistent in my preference ;)
It's also just adding one additional rule to the ignore list if we wouldn't want this change (C408, https://docs.astral.sh/ruff/rules/unnecessary-collection-call/)
There was a problem hiding this comment.
I'll defer to @jorisvandenbossche ; I don't personally have a strong opinion on this. Given that it looks like this was accepted on the GeoPandas side more for tests, and this isn't strictly for tests here, it seems like ignoring C408 might be the better option here.
There was a problem hiding this comment.
I don't have an opinion on the syntax being used. Just a very mild opinion on the principle of using an oppiniated linter to avoid this kind of dicussions and then still have them :-), but I avoid having principles in general, so no problem at all to ignore C408 ;-).
I don't know what the basis of the remark in the geopandas comment was that it would only be relevant for tests in geopandas. That is not the case: ruff runs on the entire geopandas codebase and the PR linked to also changed this in the rest of the code base.
There was a problem hiding this comment.
I was referring to this comment via the above GeoPandas link from @jorisvandenbossche re: tests:
(now, I am fine here for our tests to just follow ruff, I just wouldn't like that for my personal scripts ;))
There was a problem hiding this comment.
Yes, I know, but the PR in geopandas isn't only targetting the tests.... Obviously it didn't change @jorisvandenbossche s personal scripts though ;-)
| # controversial | ||
| "B006", | ||
| # controversial | ||
| "B007", | ||
| # controversial | ||
| "B008", |
There was a problem hiding this comment.
We might want to keep this consistent with geopandas, but I think those could probably all be removed (they don't seem that controversial, and I also doubt we have occurrences of them)
There was a problem hiding this comment.
I agree that they sound like useful checks, and some af them can even avoid weird situations that are hard to debug.
Maybe activate them in geopandas as well, then it is consistent between geopandas and pyogrio?
Same rules as applied in geopandas + pyupgrade (python 3.8) + pydocstyle