-
Notifications
You must be signed in to change notification settings - Fork 571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: DefinedNamespace: fixed handling of control attributes. #1825
fix: DefinedNamespace: fixed handling of control attributes. #1825
Conversation
8200b21
to
637e106
Compare
This, together with #1823, is needed to make docs build pass on master. |
Stack trace sample:
|
I will actually add one more test to verify that |
I think there are deeper issues here, adding more tests. |
Okay, this fix is slightly wrong, reworking it. |
Okay it seems |
f4be36b
to
d0cbbfb
Compare
_partialmethod
_NS
from __getattr__
e36f2b4
to
0a5957e
Compare
_NS
from __getattr__
_NS
attribute
42fd6e9
to
fc7267b
Compare
_NS
attribute
The remaining warning from sphinx is fixed in #1823. |
fc7267b
to
64ac2ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trickier than it looked at first.
Yes, also went down a couple of dead ends 😅 and fixed a couple of none bugs first - thanks for the review. |
This patch changes `DefinedNamespace` to always raise `AttributeError` for the name `_NS` and other "control attributes" from `__getattr__`, and to also not consider them as part of the `__dir__`. Without doign this `inspect.signature` recurses infinitely when inspecting `rdflib.namespace.DefinedNamespace` and `dir` results in an `AttributeError`. One situation in which this occurs is when sphinx autodoc is generating documentation from type hints: ``` WARNING: error while formatting signature for rdflib.namespace.DefinedNamespace: Handler <function record_typehints at 0x7fbf2696dd40> for event 'autodoc-process-signature' threw an exception (exception: maximum recursion depth exceeded while calling a Python object) ``` Also: - Changed `DefinedNamespace.__repr__` to use repr for formatting the URI string instead of quoting by hand. This probably has no real effect, as the namespace shoudl not have a double or single quote in it, but it is still correct to use repr. The main reason for this patch is to eliminate a warning for sphinx which is blocking the build since `sphinx.fail_on_warning` was enabled. There is also two cases in `DefinedNamespaceMeta` where an undefined method is being called, these were causing type errors before, but I added notes now to indicate they are real bugs, because if they are reached they just result in `AttributeError` exceptions. This should be fixed sometime, not sure what the reasoning behind it was.
64ac2ff
to
fdc6479
Compare
I will merge this tomorrow morning (CET) if there is no further feedback as this is blocking docs build and preview. |
Summary of changes
This patch changes
DefinedNamespace
to always raiseAttributeError
forthe name
_NS
and other "control attributes" from__getattr__
, and toalso not consider them as part of the
__dir__
.Without doign this
inspect.signature
recursesinfinitely when inspecting
rdflib.namespace.DefinedNamespace
anddir
results in an
AttributeError
.One situation in which this occurs is when sphinx autodoc is generating
documentation from type hints:
Also:
DefinedNamespace.__repr__
to use repr for formattingthe URI string instead of quoting by hand. This probably has no real effect,
as the namespace shoudl not have a double or single quote in it, but
it is still correct to use repr.
The main reason for this patch is to eliminate a warning for sphinx
which is blocking the build since
sphinx.fail_on_warning
was enabled.There is also two cases in
DefinedNamespaceMeta
where an undefinedmethod is being called, these were causing type errors before, but I
added notes now to indicate they are real bugs, because if they are
reached they just result in
AttributeError
exceptions. This should befixed sometime, not sure what the reasoning behind it was.
Checklist
the same change.
so maintainers can fix minor issues and keep your PR up to date.