Add new ruff lint rules #392
Conversation
nateprewitt
left a comment
nateprewitt
left a comment
There was a problem hiding this comment.
Minor formatting comments, I think we may be missing configuration for ruff in most of the projects.
| from aws_event_stream.events import Byte, EventMessage, Long, Short | ||
|
|
There was a problem hiding this comment.
These are first party imports from the same package, I'd expect it to be where it was at previously. Did we not fully configure the settings for I to recognize this as a first-party import? We may need to configure src like this.
There was a problem hiding this comment.
Yeah, we'll have to add that to each subprojects toml file if we want them to be recognized as 1p.
packages/aws-event-stream/tests/unit/_private/test_deserializers.py
Outdated
Show resolved
Hide resolved
| the `response`. If multiple `read_before_execution` methods throw exceptions, | ||
| the latest will be used and earlier ones will be logged and dropped. | ||
| """ | ||
| pass |
There was a problem hiding this comment.
Should these be raising NotImplementedError rather than just passing? Having the functions be no-ops seems odd.
There was a problem hiding this comment.
They very specifically need to be no-ops. The usage is something like
for interceptor in interceptors:
interceptor.read_before_execution(...)But not every implementation will implement all of these hooks. So if we had them throwing not implemented then we'd have to catch and ignore that, adding a bunch of useless thrashing
There was a problem hiding this comment.
Should this explicitly be return None then as a default if we're not expecting all of these to be implemented? Having a completely bare function is abnormal.
There was a problem hiding this comment.
Apparently this has already been a hotly debated topic that ruff has taken an opinionated stance on. It feels weird, but we can go with it if people feel strongly about it.
There was a problem hiding this comment.
Yeah iirc there are some that prefer pass some that prefer ... and so on. Functions have to have some sort of statement to be valid, but that statement can be the doc string. I really don't care so long as it doesn't impact performance.
haydenbaker
force-pushed
the
haydenbaker/add-new-ruff-rules
branch
from
335fd2a to
1f5b93d
Compare
| from dataclasses import dataclass | ||
| from io import BytesIO | ||
| from struct import pack, unpack | ||
| from types import MappingProxyType |
| ) | ||
|
|
||
| Document: TypeAlias = ( | ||
| type Document = ( |
There was a problem hiding this comment.
I forgot this was still here - probably needs to be deleted (that can be done later)
3 participants
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.