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

Widen Graph.__contains__ type hint #2323

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

EFord36
Copy link
Contributor

@EFord36 EFord36 commented Mar 28, 2023

Summary of changes

The typing on Graph.__contains__ doesn't currently allow rdflib.paths.Paths in the predicate position of the triple argument, even though the implementation supports this (it calls the triple method, which is type hinted to accept Paths and handles them fine). This can cause mypy errors for rdflib users on valid code. Minimal repro:

import rdflib

g = rdflib.Graph()


skos_xl_label_path = rdflib.URIRef("http://www.w3.org/2008/05/skos-xl#Label") / rdflib.URIRef("http://www.w3.org/2008/05/skos-xl#literalForm")

print((rdflib.URIRef("http://example.org/ns#bob"), skos_xl_label_path, rdflib.Literal("Bob")) in g)

gives a mypy error:

test_rdflib_typing.py:8: error: Unsupported operand types for in ("Tuple[URIRef, SequencePath, Literal]" and "Graph")  [operator]

which goes away after the change in this PR.

Occurence in my codebase here that triggers the above error (although some context is required to see why, I just wanted to point out that this was a real problem I faced and had to add a # type: ignore for, not just a hypothetical).

I also removed some commented out type hints that aren't used or referenced anywhere while I was staring at them - this looked like useful cleanup but let me know if it isn't and I'll happily remove the commit from the PR.

Thanks for rdflib, and for adding (and distributing) the type hints!

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes. (checked mypy - no new/related errors)
  • 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.

EFord36 added 2 commits March 28, 2023 15:43
These aren't used or referenced anywhere.
Paths are valid as the second argument of the triple here, as ultimately
the triples method is called, which accepts either a predicate or a
Path.
@coveralls
Copy link

Coverage Status

Coverage: 90.836%. Remained the same when pulling c7bbb64 on EFord36:widen-contains-type-hint into 081a974 on RDFLib:main.

Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request, it looks good to me. I will merge this before Monday.

@aucampia aucampia added review wanted This indicates that the PR is ready for review ready to merge The PR will be merged soon if no further feedback is provided. labels Mar 28, 2023
@aucampia aucampia requested a review from a team March 28, 2023 16:30
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

@EFord36
Copy link
Contributor Author

EFord36 commented Apr 3, 2023

It looks like there was a problem with Read the Docs when building docs, which has it stuck in 'pending' in Github. It doesn't look related to the change, and says 'please try again later':

Screenshot 2023-04-03 at 08 46 35

I don't believe I have permission to kick off the docs build again.

Is this the key blocker for merging? If so, could someone trigger the docs build again?

No major rush from my side, just wanted to make sure the docs build showing as stuck in 'pending' wasn't going to halt progress towards merging.

Thanks for your help!

@aucampia
Copy link
Member

aucampia commented Apr 3, 2023

Is this the key blocker for merging? If so, could someone trigger the docs build again?

It is unrelated, I just had a bit of a busy weekend. Merging this now.

@aucampia aucampia merged commit 1c45ec4 into RDFLib:main Apr 3, 2023
@EFord36
Copy link
Contributor Author

EFord36 commented Apr 3, 2023

It is unrelated, I just had a bit of a busy weekend. Merging this now.

ah no problem - sorry for bothering you then, didn't mean to rush you. Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants