use full name for Attributes #507
Conversation
for more information, see https://pre-commit.ci
The new commit reverted change 2, keeping the annotation of the parent object. Instead, it changed Other changes are due to the pytest does not pass on my Windows machine occasionally, as the path is case insensitive on Windows, and the case changed. |
|
I'll try to find time to review this on the weekend. |
| if ( | ||
| os.name == "nt" | ||
| ): # file path name (and drive letter) is case insensitive on windows | ||
| assert warning.list[0].filename.lower() == __file__.lower() | ||
| else: | ||
| assert warning.list[0].filename == __file__ |
There was a problem hiding this comment.
What exactly prompted this change? Was something failing?
There was a problem hiding this comment.
It break test on Windows. The drive letter (D:\) can be either lower or upper case. And they do appear differently two sides sometime.
However, they do not always appear differently. So this sometimes make the tests fail on my windows machine. I do know how to definitely reproduce this. Windows is case insensitive. Maybe os.path.normcase can be used avoid the test for OS.
| have_annotation_for_attribute = True | ||
| for seg in varpath[1:]: | ||
| if not inspect.isclass(annotation): | ||
| have_annotation_for_attribute = False | ||
| break | ||
| if sys.version_info >= (3, 10): | ||
| annotation = inspect.get_annotations(annotation).get(seg) | ||
| else: | ||
| annotation = annotation.__annotations__.get(seg) | ||
| if have_annotation_for_attribute: |
There was a problem hiding this comment.
Separate unrelated control blocks for code readability. I wish I could enforce this with Ruff.
| have_annotation_for_attribute = True | |
| for seg in varpath[1:]: | |
| if not inspect.isclass(annotation): | |
| have_annotation_for_attribute = False | |
| break | |
| if sys.version_info >= (3, 10): | |
| annotation = inspect.get_annotations(annotation).get(seg) | |
| else: | |
| annotation = annotation.__annotations__.get(seg) | |
| if have_annotation_for_attribute: | |
| have_annotation_for_attribute = True | |
| for seg in varpath[1:]: | |
| if not inspect.isclass(annotation): | |
| have_annotation_for_attribute = False | |
| break | |
| if sys.version_info >= (3, 10): | |
| annotation = inspect.get_annotations(annotation).get(seg) | |
| else: | |
| annotation = annotation.__annotations__.get(seg) | |
| if have_annotation_for_attribute: |
|
So the thing is, Typeguard so far hasn't checked attribute assignments, or at least that it didn't intend to. I haven't yet fully delved into the details of this PR, but is your intention to add checks for attribute assignments? |
Never mind, judging from the tests it looks like this was indeed your intention. But attribute assignment is much more complicated than local variable assignment due to the potential interference by descriptors and |
|
Ok, I've pushed a fix for the linked issue. Given that this PR (as I understand) expands the capabilities of Typeguard, we should take our time to sort out any potential issues with descriptors and |
|
I did wonder why the code some check for I only added type check for attributes that there is an annotation. Anything other than that are not checked. I agree with that we leave this until we know more. |
Changes
Fixes #506
In assignment check, when attribute is added:
Attributeis only handled in tuple, but not bare attribute assignments. Add check for bare attribute assignmentexp.idis used forAttribute, but forAttribute, it points to the parent object, not the attribute. Changed it to usename，which isobj.attr, the full path.Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) added which would fail without your patchdocs/, in case of behavior changes or newfeatures)
docs/versionhistory.rst).