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

Remove -Likes #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ajnelson-nist
Copy link
Contributor

@ajnelson-nist ajnelson-nist commented Apr 23, 2024

While trying to understand some of the code base's typing a bit better, I checked the definition of GraphLike and found it was not fulfilling the same role as "-like" parameters I had seen in RDFLib. This PR removes replaces types specialized to pySHACL with classes used more broadly in RDFLib. If it turns out there was another purpose these -Likes were serving, I don't mind this PR being NACKed and closed, though I'd be curious to hear about the use case.

This PR builds on PR 228. The first patch in the PR documents specific rationales on removing GraphLike and ConjunctiveLike. Some parameter type revisions around ConjunctiveGraph vs. Dataset are intentionally left out of the first patch, and I leave it to the maintainers whether they should be added to this PR.

`GraphLike` and `ConjunctiveLike` appear in several places throughout
the pySHACL code, with these definitions in a few files named
`pytypes.py`:

```python
ConjunctiveLike = Union[ConjunctiveGraph, Dataset]
GraphLike = Union[ConjunctiveLike, Graph]
```

In RDFLib 7.0.0 (and according to `git blame`, RDFLib since 2013), the
classes referenced in the `Union`s have these subclassing relationships:

* `Dataset` is a subclass of `ConjunctiveGraph`.
* `ConjunctiveGraph` is a subclass of `Graph`.

The RDFLib `Graph` definition designates a few parameters as "-likes"
(e.g., `source` in `graph.parse()`), to normalize behaviors around
graphs being passed in methods as objects or strings.  The Likes in
pySHACL do not handle the same use case pattern.

This patch replaces the -Likes with their broadest `Union` members, even
though a few methods that used `ConjunctiveLike` are described by
documentation to prefer `Dataset`.  Narrowing this is left for future
review.

Spelling of `rdflib.Graph` was kept as `rdflib.Graph` for text search
purposes.

A little cleanup occurs in this patch as well - some type-hinting in
comments is mooted by inheritance from function parameters
(`validate.py`); `/pyshacl/rdfutil/pytypes.py` is deleted by merit of
now being empty; and a `RuntimeError` message has a capitalization typo
fixed.

Signed-off-by: Alex Nelson <[email protected]>
@ashleysommer
Copy link
Collaborator

Hi @ajnelson-nist

Thanks for this. The removal of these GraphLike types was something I had been planning to get around to.

For some context, these were introduced at a time before RDFLib had any typing, this was my attempt to make sense of the Graph class hierarchy in rdflib, and make it compatible with the PySHACL typing expectations. The situation since then in RDFLib has improved, so we can probably use their types now.

Note, the solution here may not be as simple as it seems, because RDFLib is planning (has been planning for at least the last 3 years) to remove the ConjunctiveGraph class, and have Dataset inherit directly from Graph. I believe if they keep ConjunctiveGraph for compatibility reasons, it will be a subclass of Dataset. That was one reason for the ConjunctiveLike union I had included here, and your solution of simply replacing it with rdflib.ConjunctiveGraph will not suffice.

This may require further thought.

@nicholascar
Copy link
Member

ConjunctiveGraph is on the chopping block for RDFLib 8.0.0 likely due in later 2024.

Yes, I think Dataset will be a subclass of Graph but I do want to entirely remove ConjunctiveGraph, not keep it at all, just to reduce toolkit terminology and complexity.

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