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

Mismatch in PropertyIdValueImpl #27

Closed
fer-rum opened this issue Feb 27, 2014 · 5 comments
Closed

Mismatch in PropertyIdValueImpl #27

fer-rum opened this issue Feb 27, 2014 · 5 comments
Assignees

Comments

@fer-rum
Copy link
Member

fer-rum commented Feb 27, 2014

There is a mismatch in PropertyIdValueImpl between the exception message and the reason why the exception was thrown:

        if (!id.matches("^P[1-9][0-9]*$")) {
            throw new IllegalArgumentException(
                    "Wikibase item ids must have the form \"Q[1-9]+\"");
        }

Is it intentional that no property with the id "P0" is allowed?

@julianmendez
Copy link
Member

Also, the form "Q[1-9]+" does not capture Q20: "Norway".

@mkroetzsch mkroetzsch self-assigned this Feb 27, 2014
@mkroetzsch
Copy link
Member

It is intended that "0" is not allowed (that's just how things are in Wikibase; no deeper reason I guess). The exceptions text is of course wrong. Will fix.

mkroetzsch added a commit that referenced this issue Feb 27, 2014
@fer-rum
Copy link
Member Author

fer-rum commented Feb 28, 2014

Can you please add a documentation statement about the appropriate values (or add a link to the documentation), since there yet seems to be none (at least I didn't find some).
Also please note that some people consider positive integers to include 0.

@mkroetzsch
Copy link
Member

I extended the documentation in 37be7b7. I think it is generally agreed upon what "positive integer" means. At least I found the same definition in all places I looked. If 0 is included, one usually speaks of non-negative integers.

fer-rum pushed a commit that referenced this issue Feb 28, 2014
@fer-rum
Copy link
Member Author

fer-rum commented Feb 28, 2014

I think that will do it.

@fer-rum fer-rum closed this as completed Feb 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants