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

Use the property built-in as a decorator #1703

Merged

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Feb 1, 2022

The use of the property built-in as a function does not allow for
the same type annotations as using property as a decorator.

This change should not affect runtime behaviour, but I added tests to
cover all properties that were changed to decorators explicitly just to
make sure they work as they should.

There are still other places where the property built-in is used as a
function but this is outside of the core parts of RDFLib.


For some context, I'm busy adding typing to store/queries and without this things get a bit hairy. I'm doing this separately as this goes beyond simple addition of type annotations, and as such want this to stand on it's own to make it easier to review and asses.

@aucampia aucampia force-pushed the iwana-20220130T2327-modern_property branch from 8cb5df8 to 0d2c762 Compare February 1, 2022 23:50
@aucampia
Copy link
Member Author

aucampia commented Feb 2, 2022

Actually I'm going to add more tests first, all lines are covered by tests but there are no explicit tests for these properties.

@aucampia aucampia marked this pull request as draft February 2, 2022 00:00
@aucampia aucampia force-pushed the iwana-20220130T2327-modern_property branch 3 times, most recently from ca12e19 to ca6ae6e Compare February 2, 2022 00:42
@aucampia
Copy link
Member Author

aucampia commented Feb 2, 2022

Actually I'm going to add more tests first, all lines are covered by tests but there are no explicit tests for these properties.

added now.

@aucampia aucampia marked this pull request as ready for review February 2, 2022 00:45
@aucampia aucampia force-pushed the iwana-20220130T2327-modern_property branch from ca6ae6e to 6fd0857 Compare February 2, 2022 01:01
@aucampia aucampia added review wanted This indicates that the PR is ready for review good first issue Good for newcomers help wanted Extra attention is needed labels Feb 6, 2022
The use of the `property` built-in as a function does not allow for
the same type annotations as using `property` as a decorator.

This change should not affect runtime behaviour, but I added tests to
cover all properties that were changed to decorators explicitly just to
make sure they work as they should.

There are still other places where the `property` built-in is used as a
function but this is outside of the core parts of RDFLib.
@aucampia aucampia force-pushed the iwana-20220130T2327-modern_property branch from 6fd0857 to f0a16af Compare February 8, 2022 22:07
Copy link
Member

@nicholascar nicholascar left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me!

@aucampia aucampia merged commit 4eba1a0 into RDFLib:master Feb 12, 2022
@aucampia aucampia deleted the iwana-20220130T2327-modern_property branch April 9, 2022 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants