Skip to content
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

Issue 2283: Add type-review support for SPARQL query results from Graph.query #2320

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ajnelson-nist
Copy link
Contributor

@ajnelson-nist ajnelson-nist commented Mar 27, 2023

Summary of changes

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
    • It's possible PR 2301 and/or PR 2267 will have some impact on this implementation. That's a guess from seeing the wide-ranging set of type signature complaints encountered in this PR's initial patch.
  • Added tests for any changes that have a runtime impact.
    • Added tests for SELECT and CONSTRUCT queries
    • Added tests for DESCRIBE and ASK queries
  • Checked that all tests and type checking passes. (Submitter: Known to not pass yet; will pass before PR is requested to be merged.)
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
    • Considered adding an example in ./examples for new features.
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

This patch is known to be an incomplete implementation.  Running
`mypy test/test_typing.py` before this patch raised 0 type errors.
After just adding the two new tests, 3.  After starting the subclasses
of Query, 30.

References:
* RDFLib#2283

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@aucampia
Copy link
Member

PRs to V6 is closed until further notice. See this for more details:

@aucampia aucampia added the on hold Progress on this issue is blocked by something. label May 21, 2023
@aucampia
Copy link
Member

PRs to V6 is closed until further notice. See this for more details:

We will be open for PRs again once this is resolved:

@aucampia aucampia removed the on hold Progress on this issue is blocked by something. label Jun 2, 2023
@nicholascar
Copy link
Member

@ajnelson-nist do you want to continue with this PR not that v7.x is out and we are merging PRs again?

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
Potentially, adding an assert() addresses the subclass type review,
per `@classmethod` docs:

> ... If a class method is called for a derived class, the derived class
> object is passed as the implied first argument.

This patch also reverts the first-draft change to `Graph.query`.

References:
* https://docs.python.org/3/library/functions.html#classmethod
* RDFLib#2283 (comment)

Suggested-by: Iwan Aucamp <aucampia@gmail.com>
Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
References:
* RDFLib#2283

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist
Copy link
Contributor Author

I've given this a nudge. Following @aucampia 's suggestion looks promising, but it runs into a circular import issue bringing in prepareQuery at the point he'd suggested.

The class rdflib.query.Result caught my eye as a potential next point of inspection.

@ajnelson-nist
Copy link
Contributor Author

I'm pausing working on this for the time being. Suggestions are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants