Tighten up ruff ignore list #18837
Tighten up ruff ignore list #18837
Conversation
Quexington
added
Changed
ruff.toml
Outdated
| "PLC0414", # useless-import-alias | ||
| "PLC0105", # type-name-incorrect-variance (2) (?) | ||
| "PLC0415", # import-outside-top-level (286) (x) | ||
| "PLC2801", # unnecessary-dunder-call (11) (x) |
There was a problem hiding this comment.
If there's a non-dunder alternative then I would expect we should use that? Like type() vs. .__class__. Necessary ones are like .__name__ where there's no helper alternative.
There was a problem hiding this comment.
The reason this is a "good" ignore for me is because we often use object.__setattr__ to edit frozen dataclasses and while streamable exists, I'm not sure that's going away.
ruff.toml
Outdated
| "PLC0415", # import-outside-top-level (286) (x) | ||
| "PLC2801", # unnecessary-dunder-call (11) (x) | ||
| "PLC1901", # compare-to-empty-string (51) (x) | ||
| "PLC2701", # import-private-name (24) (x) |
There was a problem hiding this comment.
Seems like generally we should think this through. There are various caveats and decisions about what practices to use, but I also think there are likely many cases where we import private stuff that either shouldn't be private or we shouldn't import it.
There was a problem hiding this comment.
Yeah, that's probably fair. A lot of the ignores here are stuff in the tools directory importing from chia._tests though which is why I marked it as such.
| "PLR0915", # too-many-statements (222) (x) | ||
| "PLR0914", # too-many-locals (328) (x) | ||
| "PLR0913", # too-many-arguments (397) (x) | ||
| "PLR0912", # too-many-branches (193) (x) | ||
| "PLR1702", # too-many-nested-blocks (124) (x) | ||
| "PLR0904", # too-many-public-methods (47) (x) | ||
| "PLR0917", # too-many-positional-arguments (345) (x) | ||
| "PLR0916", # too-many-boolean-expressions (3) (x) | ||
| "PLR0911", # too-many-return-statements (52) (x) |
There was a problem hiding this comment.
I mean... would be nice to enforce this but I would at least agree that's an extra long term goal.
There was a problem hiding this comment.
It just all struck me as too arbitrary. We're not used to linting suggestions that aren't based on concrete benefits around here. Also, seems like it would lead to a lot of # noqa I feel like.
| @@ -2086,11 +2086,11 @@ async def create_offer_for_ids( | |||
| driver_dict[bytes32.from_hexstr(key)] = PuzzleInfo(value) | |||
|
|
|||
| modified_offer: dict[Union[int, bytes32], int] = {} | |||
| for key in offer: | |||
| for key, val in offer.items(): | |||
There was a problem hiding this comment.
For these in general I would request a consideration of more specific names for each case as useful. And if nothing more specific... I would personally prefer a non-abbreviated value. :] The latter is just a mild suggestion, the former a request.
| @@ -880,7 +880,7 @@ def _map_sub_epoch_summaries( | |||
| if idx < len(sub_epoch_data) - 1: | |||
| delta = 0 | |||
| if idx > 0: | |||
There was a problem hiding this comment.
Side note, when would this not be true?
| @@ -880,7 +880,7 @@ def _map_sub_epoch_summaries( | |||
| if idx < len(sub_epoch_data) - 1: | |||
| delta = 0 | |||
| if idx > 0: | |||
| delta = sub_epoch_data[idx].num_blocks_overflow | |||
| delta = data.num_blocks_overflow | |||
There was a problem hiding this comment.
caveat on this bit since we are iterating pairwise with the [idx + 1] indexing below. would be nice to clean up that iteration so what we're doing is described right at the for.
i'll just let this do the thing ruff wants here since both before and after we both use data and index using idx.
| @@ -14,7 +14,7 @@ | |||
| from typing import Any, Optional, overload | |||
| from urllib.parse import urlparse | |||
|
|
|||
| import boto3 as boto3 | |||
| import boto3 | |||
There was a problem hiding this comment.
I presume we don't want it here, but note that this does trigger mypy to consider it as exported so it isn't entirely without 'purpose'. Glad to see it removed here though.
| @@ -47,3 +47,6 @@ def replace_str_to_bytes(constants: ConsensusConstants, **changes: Any) -> Conse | |||
|
|
|||
| # TODO: this is too magical here and is really only used for configuration unmarshalling | |||
| return constants.replace(**filtered_changes) # type: ignore[arg-type] | |||
|
|
|||
|
|
|||
| __all__ = ["ConsensusConstants", "replace_str_to_bytes"] | |||
There was a problem hiding this comment.
We don't do this much so I would tend to just leave the existing 'trick' with an ignore. I need to talk to folks about getting rid of this sort of chia_rs re-exporting though in general. I think that was a transitionary plan to reduce the size of some diffs. If so, we can remove and deal with each case one by one to get this tidied.
ruff.toml
Outdated
| "PLW1641", # eq-without-hash (5) (?) | ||
| "PLW1514", # unspecified-encoding (69) (?) | ||
| "PLW0602", # global-variable-not-assigned (5) (?) | ||
| "PLW0603", # global-statement (9) (?) |
There was a problem hiding this comment.
I think we ought to usually avoid this. I have a few cases I remember offhand and some are ok and some I think we shouldn't have so I think the extra attention put on future global statements would be good.
No description provided.