-
Notifications
You must be signed in to change notification settings - Fork 555
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
Make unregister_custom_function idempotent #1659
Conversation
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.
The change looks good, this changes public API, but I don't really think it is a massive problem in this case. I think the behaviour with this patch is more desirable.
rdflib/plugins/sparql/operators.py
Outdated
@@ -613,8 +614,9 @@ def decorator(func): | |||
|
|||
def unregister_custom_function(uri, func): |
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.
One thing that maybe bothers me a bit, what if you just want to unregister a custom function regardless of the action function identity. Maybe if func is none the check for function identity should not be performed.
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.
We still have to check for membership inside the dictionary, else we will get a KeyError
when deleting a non-existent function. Maybe something like this would work?
def unregister_custom_function(uri, func):
if func is not None and _CUSTOM_FUNCTIONS.get(uri, (None, None))[0] != func:
warnings.warn("This function is not registered as %s" % uri.n3())
elif uri in _CUSTOM_FUNCTIONS:
del _CUSTOM_FUNCTIONS[uri]
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.
yes, but lets wait for @nicholascar to give some input, I tagged him for review. I still the the change as is makes sense, but if we are going to change the function we can maybe go a bit further, even if not it is good to discuss some options.
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.
Hmmm... I didn't realise func
was in the function signature. I'm happy with this PR's update and will approve but I do think it would be better to be able to de-register a function based just on uri
as a user shouldn't need to know the Python function to remove the SPARQL function.
We still have to check for membership inside the dictionary, else we will get a
KeyError
Sure, but we can do this without knowing about func
:
def unregister_custom_function(uri, func=None):
if _CUSTOM_FUNCTIONS.get(uri):
del _CUSTOM_FUNCTIONS[uri]
else:
warnings.warn("This function is not registered as %s" % uri.n3())
I think the test _CUSTOM_FUNCTIONS.get(uri, (None, None))[0] != func
is a waste of time. Firstly it may as well just be _CUSTOM_FUNCTIONS.get(uri) != func
as this yeilds the same test and secondly, who cares? Just deregister whatever uri
is as that's the guarenteed unique custom function key.
If we just use func=None
then we can implement the above and it people have legacy code parameterising the function with uri
& func
it will still work.
ASIDE: the function names should probably be deregister_custom_function
and that seems to be more grammatically corect to me to describe the function's positive action of removing a registered function! Too late for this...
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.
I agree, and thanks for the perspective. I've put your simplified implementation in my PR and adjusted the tests to match the new behavior
Thanks @gtfierro |
Fixes #1492
Proposed Changes
unregister_custom_function
can now be called multiple times on the same function (idempotent)issue 274
test by checking that the warning is raised, rather than the exception