-
Notifications
You must be signed in to change notification settings - Fork 554
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
Streamline typing and implementation of SPARQL processor #2301
Conversation
@cthoyt thanks for the PR, I'm only going to be merging bugfixes until next week as I want to release 6.3.2 with some regression fixes, but I will review this when I have time. |
Thanks @aucampia. I hope some of my recent PRs are more helpful than stressful for you. Please take your time, and thank you for all of your efforts. Cheers |
rdflib/query.py
Outdated
initBindings: Optional[Mapping["str", "Identifier"]] = None, # noqa: N803 | ||
initNs: Optional[Mapping[str, Any]] = None, # noqa: N803 |
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.
While the current function signature is problematic due to its use of mutable data structures in argument defaults, One problem with changing this is, plugins that implement this as it stands could break if they now receive valid values according to the new function signature, as they don't expect None
as a value.
We need to figure out a way to upgrade our plugin interfaces, I think the right approach is to create ProcessorV2, but we need to also consider what other changes we want to introduce before that.
I think it is best to change this back to what it was, though the rest of the PR seems good and quite helpful.
initBindings: Optional[Mapping["str", "Identifier"]] = None, # noqa: N803 | |
initNs: Optional[Mapping[str, Any]] = None, # noqa: N803 | |
initBindings: Mapping["str", "Identifier"] = {}, # noqa: N803 | |
initNs: Mapping[str, Any] = {}, # noqa: N803 |
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 see how this could be an issue. I will switch the defaults to retain the empty dictionary
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.
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 don't think 259c10d is really needed, I think it is good to keep it as you had it, because those are not part of our plugin interface.
The only part of your change that relates to the interface is the change to rdflib.query.Processor
, and here the most critical thing is that we don't want other parts of RDFLib to call plugins in a way or with arguments that they don't support, so if RDFLib code calls rdflib.query.Processor.query
with None
value for initBindings or initNs then that should result in a type error from mypy.
I will revert 259c10d I think, and then remove the optionals in rdflib.query.Processor
, I will ping you when it is done so you can just confirm that you are happy with it.
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.
Done now, please have a look and let me know if things look okay, but essentially I just reverted 259c10d and then changed rdflib/query.py
back to what is in the main branch.
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 PR looks good, but out of an abundance of caution, we should probably keep the signature of rdflib.query.Processor.query
the same as it was.
This reverts commit 259c10d.
`rdflib.query.Processor` is an interface for plugins, and changes to it that broaden it will affect existing plugins that only support the narrower interface.
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.
Looks good to me now, only change is essentially restoring rdflib/query.py
to the state from the main branch.
Summary of changes
This PR first removes situations where a mutable object (i.e., a dictionary) was used as the default value for functions in the
rdflib.plugins.sparql.processor
module and related code. It replaces these situations with the an "optional" that defaults to None, and is then handled within the function. Luckily, some of the code that the SPARQL Processor relied on already had this style, meaning not a lot of changes had to be made.Second, this PR makes a small update to the logic in the SPARQL Processor's query function to simplify the if/else statement. This better mirrors the implementation in the UpdateProcessor
These changes should not affect any user-level code and should already be covered by existing tests. The relaxation of the typing should not affect any downstream type checking.
Checklist
the same change.
so maintainers can fix minor issues and keep your PR up to date.