Skip to content

Do not crash UI for "unknown" calendar models #62

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

Closed
wants to merge 1 commit into from
Closed

Conversation

thiemowmde
Copy link
Contributor

This patch does two things:

  1. Lazy detection of the Julian calendar model. This will, for example, detect http://www.wikidata.org/wiki/Q1985786 (note the word "wiki" instead of "entity") and do the right(tm) thing.
  2. All other URLs are ignored and assumed to be Gregorian.

This probably conflicts with #61 by @snaterlicious, but I wanted to store it so we don't forget.

I can split this into two patches, if this helps in getting this merged faster.

Bug: T89698

@snaterlicious
Copy link
Contributor

Please, no. If I perceive the problem correctly, it should not be worked around like that. There seems to be something seriously broken on how calendar models are referenced. There should be one dedicated "ID" for each calendar model (which happens to be the Wikidata Item ID encapsulated by a dedicated URI which is acting like an ID). Apart from the fact that having an URI as ID might be kind of odd as the Item it references can be access using different URIs, are there going to be multiple references (URIs) to single calendar models in the data base then?

@thiemowmde
Copy link
Contributor Author

Are you referring to 1. or 2. or both?

I'm not saying this is the best fix, but the fact that this just crashes the whole UI is a bug and needs a fix. Pinging @lydiapintscher.

@snaterlicious
Copy link
Contributor

(1) is more of a conceptual issue and I sincerely doubt it belongs into the code in any way. (2) should indeed be fixed. However, not by removing throwing an error but rather by catching an resolving it. That should be done on a higher level (in Wikibase) as the code here is not supposed to make fall-back assumptions. The error is bubbling up to jQuery.snakview.variations.Value._setValue() and on to jQuery.wikibase.snakview.variations.Variation.value(). One of these functions should wrap the call causing the error in a try-catch block.

@brightbyte
Copy link

@snaterlicious: is an unknown calendar model actually an error? To me, it's just a non-default cause, one we don't provide pretty UI for. But it's perfectly valid. Otherwise, the API should reject it (and even if the API started to reject them, we may still get them from the database).

@adrianheine
Copy link
Contributor

@brightbyte It's currently an error because we cannot even represent them. It should not be an error, just as non-default precision values are not an error.

@thiemowmde
Copy link
Contributor Author

Discussion moved to https://phabricator.wikimedia.org/T89698#1088683.

@thiemowmde thiemowmde closed this Mar 4, 2015
@thiemowmde thiemowmde deleted the calCrash branch March 4, 2015 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants