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

Allow duplicates in rdf:List #690

Merged
merged 3 commits into from
Jan 19, 2017
Merged

Allow duplicates in rdf:List #690

merged 3 commits into from
Jan 19, 2017

Conversation

wwaites
Copy link
Member

@wwaites wwaites commented Jan 19, 2017

The check causing the problem in #223 is incorrect. This addresses the first half of the issue, allowing duplicates in lists. It does not address closing the list.

@gromgull
Copy link
Member

The closing the list issue is less important. It's now always closed. Leaving an open list is a very special case, people who want that can probably fix it themselves.

@wwaites
Copy link
Member Author

wwaites commented Jan 19, 2017

Coveralls seems a bit confused. Removing code and adding a test should only increase test coverage!

@gromgull
Copy link
Member

Actually - I find coveralls almost completely useless - we should just remove it :)

@wwaites
Copy link
Member Author

wwaites commented Jan 19, 2017

Seems so. Anyhow, are you satisfied that this is the right thing to do? Should I merge it?

@gromgull
Copy link
Member

gromgull commented Jan 19, 2017

The check was added to avoid an infinite loop in cases of degenerate lists I guess, but it's totally misguided, since it checks for duplicate values, not duplicate list elements.

The correct (?) test is here: https://github.com/RDFLib/rdflib/blob/master/rdflib/graph.py#L801-L802

The whole __len__ method could then just be len(list(self.graph.items(self.uri))) ?

@wwaites
Copy link
Member Author

wwaites commented Jan 19, 2017

Yes, you're correct of course. Would have been obvious if I had changed the test to do

assert len(c) == 3

now done.

@wwaites wwaites merged commit c8bc3f6 into RDFLib:master Jan 19, 2017
@gromgull
Copy link
Member

👍 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in-resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants