Switch from mypy to ty#465
Conversation
Redux of #418 - `ty` is still iterating, but now used over in connect-python (connectrpc/connect-python#242); seems reasonable to be consistent across our projects.
| def _is_repeated(field: descriptor.FieldDescriptor) -> bool: | ||
| if hasattr(field, "is_repeated"): | ||
| return field.is_repeated | ||
| return field.label == descriptor.FieldDescriptor.LABEL_REPEATED |
There was a problem hiding this comment.
minor refactor to avoid needing a ty: ignore
There was a problem hiding this comment.
I'm not familiar yet with what parts would significantly affect perf but I could see wanting to keep this boolean to a one-time check.
For what it's worth, I found this passes, up to you
# protobuf 7+ removed FieldDescriptor.label / LABEL_REPEATED in favour of is_repeated.
_FieldDescriptorClass = descriptor.FieldDescriptor
if hasattr(_FieldDescriptorClass, "is_repeated"):
def _is_repeated(field: descriptor.FieldDescriptor) -> bool:
return field.is_repeated # type: ignore[attr-defined]
else:
def _is_repeated(field: descriptor.FieldDescriptor) -> bool:
return field.label == descriptor.FieldDescriptor.LABEL_REPEATED # type: ignore[attr-defined]There was a problem hiding this comment.
done in 2e8a9ad; seems like I didn't even need ignores? (I think without the initial _FieldDescriptorClass assignment it did when I checked, hence this formulation, but agreed that perf matters more than satisfying the type-checker, and glad we could have our cake and eat it too!)
| def __getitem__(self, name): | ||
| field = self.desc.fields_by_name[name] | ||
| if field.has_presence and not self.msg.HasField(name): | ||
| def __getitem__(self, key): |
There was a problem hiding this comment.
ty wants names to match up in overrides; done several places here.
| """The rules associated with a single 'rules' message.""" | ||
|
|
||
| def validate(self, ctx: RuleContext, _: message.Message): | ||
| def validate(self, ctx: RuleContext, message: message.Message): # noqa: ARG002 |
There was a problem hiding this comment.
unused, but ty wants the name to match up. I think we could make this behave just fine with a larger refactor to make one of these an ABC, but probably not worth pursuing here?
There was a problem hiding this comment.
Agree it seems like it should be an ABC but can be separate
| def validate_item(self, ctx: RuleContext, val: typing.Any, *, for_key: bool = False): | ||
| self._validate_value(ctx, val, for_key=for_key) | ||
| self._validate_cel(ctx, this_value=val, this_cel=_scalar_field_value_to_cel(val, self._field), for_key=for_key) | ||
| def validate_item(self, ctx: RuleContext, value: typing.Any, *, for_key: bool = False): |
There was a problem hiding this comment.
val -> value (hard to spot at first)
| def _is_repeated(field: descriptor.FieldDescriptor) -> bool: | ||
| if hasattr(field, "is_repeated"): | ||
| return field.is_repeated | ||
| return field.label == descriptor.FieldDescriptor.LABEL_REPEATED |
There was a problem hiding this comment.
I'm not familiar yet with what parts would significantly affect perf but I could see wanting to keep this boolean to a one-time check.
For what it's worth, I found this passes, up to you
# protobuf 7+ removed FieldDescriptor.label / LABEL_REPEATED in favour of is_repeated.
_FieldDescriptorClass = descriptor.FieldDescriptor
if hasattr(_FieldDescriptorClass, "is_repeated"):
def _is_repeated(field: descriptor.FieldDescriptor) -> bool:
return field.is_repeated # type: ignore[attr-defined]
else:
def _is_repeated(field: descriptor.FieldDescriptor) -> bool:
return field.label == descriptor.FieldDescriptor.LABEL_REPEATED # type: ignore[attr-defined]| """The rules associated with a single 'rules' message.""" | ||
|
|
||
| def validate(self, ctx: RuleContext, _: message.Message): | ||
| def validate(self, ctx: RuleContext, message: message.Message): # noqa: ARG002 |
There was a problem hiding this comment.
Agree it seems like it should be an ABC but can be separate
2 participants
Redux of #418 -
tyis still iterating, but now used over in connect-python (connectrpc/connect-python#242); seems reasonable to be consistent across our projects.