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

Return null in getBestStatements if there are none #432

Merged
merged 3 commits into from Aug 28, 2019

Conversation

bennofs
Copy link
Contributor

@bennofs bennofs commented Aug 28, 2019

StatementGroup requires at least one statement, so calling
getBestStatements if there are no best statements previously would throw
a "non-empty list of statements must be provided" validation error.


Another solution may be to remove the assertation that statement groups cannot be empty.
Then we could return an empty statement group instead of null.

Having no best statement can happen if there is only a deprecrated
statement in the statement group.

StatementGroup requires at least one statement, so calling
getBestStatements if there are no best statements previously would throw
a "non-empty list of statements must be provided" validation error.

Having no best statement can happen if there is only a deprecrated
statement in the statement group.
@coveralls
Copy link

coveralls commented Aug 28, 2019

Coverage Status

Coverage increased (+0.002%) to 80.436% when pulling e7bbf9e on bennofs:fix-best-statements-none into 77d5684 on Wikidata:master.

@wetneb
Copy link
Member

wetneb commented Aug 28, 2019

Looks sensible to me! I would just mention that the method can return null in its documentation (especially in the interface StatementGroup).

@bennofs
Copy link
Contributor Author

bennofs commented Aug 28, 2019

@wetneb thanks, changed the documentation to mention that

@Tpt Tpt merged commit c87ebf4 into Wikidata:master Aug 28, 2019
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.

None yet

4 participants