chore: Adopting more ruff complexity rules#2460
Conversation
- #2376 - https://docs.astral.sh/ruff/rules/complex-structure - Mainly `hist`, `dtype` conversion, `__getitem__`, `group_by`, `over`, `join`
Excluded backend `utils`, too much overlap with `C901`
|
Thanks @dangotbanned - I am a bit perplexed about this one as of now, mostly due to
This does not mean I am against it, but as of now I don't see the benefits nor what it would imply to adopt them
Could you maybe show how one of those According to such example it might be easier for everyone to evaluate how the future might look like 😁 |
|
Sure thing @FBruzzesi I'll take a stab at one tomorrow! The aim of the limits set by the rules are to nudge you when something you're writing is starting to get difficult for others to understand. |
|
|
||
| @functools.lru_cache(maxsize=16) | ||
| def non_object_native_to_narwhals_dtype(native_dtype: Any, version: Version) -> DType: | ||
| def non_object_native_to_narwhals_dtype(native_dtype: Any, version: Version) -> DType: # noqa: C901, PLR0912 |
There was a problem hiding this comment.
This could benefit from something like the example in (PLR0911)
PANDAS_LIKE_TO_NW_PRIMITIVE
PANDAS_LIKE_TO_NW_PRIMITIVE = {
"Int64": "Int64",
"Int64[pyarrow]": "Int64",
"int64": "Int64",
"int64[pyarrow]": "Int64",
"Int32": "Int32",
"Int32[pyarrow]": "Int32",
"int32": "Int32",
"int32[pyarrow]": "Int32",
"Int16": "Int16",
"Int16[pyarrow]": "Int16",
"int16": "Int16",
"int16[pyarrow]": "Int16",
"Int8": "Int8",
"Int8[pyarrow]": "Int8",
"int8": "Int8",
"int8[pyarrow]": "Int8",
"UInt64": "UInt64",
"UInt64[pyarrow]": "UInt64",
"uint64": "UInt64",
"uint64[pyarrow]": "UInt64",
"UInt32": "UInt32",
"UInt32[pyarrow]": "UInt32",
"uint32": "UInt32",
"uint32[pyarrow]": "UInt32",
"UInt16": "UInt16",
"UInt16[pyarrow]": "UInt16",
"uint16": "UInt16",
"uint16[pyarrow]": "UInt16",
"UInt8": "UInt8",
"UInt8[pyarrow]": "UInt8",
"uint8": "UInt8",
"uint8[pyarrow]": "UInt8",
"Float64": "Float64",
"Float64[pyarrow]": "Float64",
"double[pyarrow]": "Float64",
"float64": "Float64",
"float64[pyarrow]": "Float64",
"Float32": "Float32",
"Float32[pyarrow]": "Float32",
"float32": "Float32",
"float32[pyarrow]": "Float32",
"float[pyarrow]": "Float32",
"large_string[pyarrow]": "String",
"string": "String",
"string[pyarrow]": "String",
"string[python]": "String",
"bool": "Boolean",
"bool[pyarrow]": "Boolean",
"boolean": "Boolean",
"boolean[pyarrow]": "Boolean",
}We can then reduce the first 12 branches into a single case
@functools.lru_cache(maxsize=16)
def non_object_native_to_narwhals_dtype(native_dtype: Any, version: Version) -> DType: # noqa: C901, PLR0912
dtype = str(native_dtype)
dtypes = version.dtypes
if normalized := PANDAS_LIKE_TO_NW_PRIMITIVE.get(dtype):
return getattr(dtypes, normalized)()
elif ...
narwhals/_arrow/series_dt.py
Outdated
| return self.with_native(ser.native.cast(pa.timestamp(self.unit, time_zone))) | ||
|
|
||
| def timestamp(self, time_unit: TimeUnit) -> ArrowSeries: | ||
| def timestamp(self, time_unit: TimeUnit) -> ArrowSeries: # noqa: C901, PLR0912 |
There was a problem hiding this comment.
Note
TODO Done
Okay I'll do this one in this PR - have been wanting to refactor it for a while anyway
There was a problem hiding this comment.
@FBruzzesi first worked example in (6666fe2)
I could have reduced the LOC as well, but I think the typing helps reduce the amount of info you need to keep in your head 🙂
There was a problem hiding this comment.
Yep that's great (as commented here) 🙌🏼 rethinking functorialities in this way is pretty helpful for long term maintenance!
I would propose to:
- Wrap up this PR as is (or any minor adjustment if needed)
- Keep [internal]: Adopting/following more
ruffcomplexity rules #2376 as tracker for the functionalities that had to add#noqa: <ANY NEW RULE>or open a new issue for it so that we can progressively tackle them
There was a problem hiding this comment.
Thanks @FBruzzesi - I've finished moving all of them into child issues of
There was a problem hiding this comment.
I am already sold from this diff 👌🏼😉
dangotbanned
changed the title
ruff complexity rulesruff complexity rules
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned 👌🏼 Happy to ship 🚢 so that we can start to address all methods
2 participants
Will close #2376
What type of PR is this? (check all applicable)
Related issues
ruffcomplexity rules #2376Checklist
If you have comments or can explain your changes, please do so below
Important
Currently just highlighting areas that need some love
Would need some buy-in from others that they'd like to see this stuff simplified/broken up 🙂