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.

Sign up for GitHub

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

Jump to bottom

fix: bugs with rdflib.extras.infixowl #2390

Merged
merged 25 commits into from
May 19, 2023
Merged

fix: bugs with rdflib.extras.infixowl #2390

merged 25 commits into from
May 19, 2023

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented May 19, 2023 •
edited
Loading

Summary of changes

Fix the following issues in rdflib.extras.infixowl:

  • getting and setting of max cardinality only considered identifiers and not other RDF terms.
  • The return value of manchesterSyntax was wrong for some cases.
  • The way that BooleanClass was generating its string representation (i.e. BooleanClass.__repr__) was wrong for some cases.

Other changes:

  • Added an example for using infixowl to create an ontology.
  • Updated infixowl tests.
  • Updated infixowl documentation.

This code is based on code from:

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.
  • 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 granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

Graham Higgins and others added 24 commits March 22, 2023 13:50
docstring updates
9db3c56 
tidy `# docstring` syntax and replace antiquated namespace_manager bindings with moderm graph ns bindings.
change to use @ as infix operator
359d69f
Add documentation to Individual, rename serialize.
4e8301e
modernize OWLRDFListProxy class def, add _rdfList field.
4069831
Robustify return of string
0bb25a3
Add documentation of operator mapping
0aac6a6
Fix Callable issue
45fc1eb
Fix cardinality issue
da93538
Change assertions to exceptions
c6fb13f
Add _rdfList class variable, revert Exceptions back to AssertionErrors.
80b1c7d
update infixowl tests
c89bc79
Add example of ontology creation using infixowl syntax
7861598
fix isort issues
2ea0763
busywork
2da73e9
revert ill-conceived field addition
1e61070
@aucampia
Merge branch 'main' into infixowl-fix
d948c42
@aucampia
remove xfails that are not needed
39e33d8
Merge remote-tracking branch 'origin/main' into infixowl-fix
9e77f35
Minor changes as per RDFLib#2307 (review)
febf6bd
@aucampia
Merge branch 'main' into infixowl-fix
44dbc16
@aucampia
Merge branch 'main' into infixowl-fix
c3b772b
@aucampia
Remove new functions that don't have tests yet
1b9ca4a 
`subject_node_closure` and `blank_node_closure` don't have tests and are
not essential to the rest of the PR. Removing them so the rest can be
merged.
@aucampia
Add back Individual.serialize
5130e36 
Removing this would be a breaking change, it should be marked as
deprecated instead and if it is being replaced by another function then
it should call the other function.
@aucampia
Add raises to xfail
23403d2 
This will invalidate the xfail if the exception being raised changes,
which makes it a bit safer to use xfail, as it is less likely to mask
other newly introduced bugs.
@aucampia
Copy link
Member Author

aucampia commented May 19, 2023 •
edited
Loading

@gjhiggins please see if you are okay with the changes I made. All changes can be seen here:

Of particular import may be:

  • 5130e36

    Add back Individual.serialize

    Removing this would be a breaking change, it should be marked as
    deprecated instead and if it is being replaced by another function then
    it should call the other function.

aucampia
aucampia commented May 19, 2023
View reviewed changes
rdflib/extras/infixowl.py Outdated Show resolved Hide resolved
@aucampia aucampia marked this pull request as ready for review May 19, 2023 08:53
@aucampia aucampia requested review from a user May 19, 2023 08:53
@aucampia aucampia added the review wanted This indicates that the PR is ready for review label May 19, 2023
@coveralls
Copy link

coveralls commented May 19, 2023 •
edited
Loading

Coverage Status

Coverage: 90.85% (-0.004%) from 90.854% when pulling e65e721 on aucampia:aucampia/20230519T1021-infix-fixes into e103078 on RDFLib:main.

ghost
ghost approved these changes May 19, 2023
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

👍 (and thank you)

@aucampia
Change Callable.__callfn__ to Callable._callfn
e65e721 
As far as I know this is not a documented dunder, and according to
PEP-8
\[[ref](https://peps.python.org/pep-0008/#descriptive-naming-styles)\]:

> Never invent such names; only use them as documented.
@aucampia aucampia force-pushed the aucampia/20230519T1021-infix-fixes branch from f99f085 to e65e721 Compare May 19, 2023 10:05
@aucampia aucampia added the ready to merge The PR will be merged soon if no further feedback is provided. label May 19, 2023
@aucampia aucampia merged commit cd0b442 into RDFLib:main May 19, 2023
@aucampia aucampia mentioned this pull request May 19, 2023
Closed
8 tasks
@aucampia aucampia deleted the aucampia/20230519T1021-infix-fixes branch June 8, 2023 17:49
This was referenced Jul 6, 2023
Open
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aucampia @coveralls