Remove weirdness from Fingerprint methods #40
Conversation
|
o_O How can the tests actually pass after this breaking change? There must be something really wrong with the tests if they don't catch this up... |
|
I do support the change. But as you said this needs more work and investigation. Especially: Where are these methods used? |
|
Wondering about usage as well. We do not have a hasLabel that takes a Term or a combination of language and text on the PHP side. Only hasFoo methods that take a language code. |
| * @param {wikibase.datamodel.Term} label | ||
| * @return {boolean} | ||
| */ | ||
| hasLabel: function( languageCode, label ) { | ||
| return this._labels.hasItem( languageCode, label ); | ||
| hasLabel: function( label ) { |
There was a problem hiding this comment.
As @JeroenDeDauw said this should be hasLabel: function( languageCode ).
There was a problem hiding this comment.
Note that we already have hasLabelFor below which does exactly that. We could just remove this method but I think it is useful if you don't only want to check if a label is set but also compare it to the value passed here.
|
This patch removes the first
The inconsistent code in AliasesChanger vs. DescriptionsChanger vs. LabelsChanger is really, really weird:
We talked about this and we think it's better to make the JavaScript DataModel immutable, mostly to get rid of unused code and complexity. Most (but not all) of this patch will be obsolete then. |
thiemowmde
force-pushed
the
fingerprint-weirdness
branch
from
a15bb45 to
931c3a8
Compare
|
I rebased this after #43/#59, updated the tests and did a little bit of a revert: I kept the error thrown in This "weirdness" was introduced in 73047ba, which was part of #16. I can not tell what the intention was. Possibly something with language fallbacks, where a I would like to have this in the 3.0 release. |
|
Will rebase and fix the issues |
|
It's rebased already. Sorry. I did not know you are working on this right now. |
|
How does this interact with fallback terms? |
|
Good question. At the moment it does not "interact" at all, because these methods are all completely unused (1 exception, see above). In my opinion the design of these methods is bad and should be streamlined as this patch proposes. There is no need to "support" fallbacks at this level. Almost all users of these functions will use them like this: fingerprint.setLabel( label.getLanguageCode(), label );It's completely pointless to even have the additional parameter then. It does not help protecting us from anything. A proper solution would be an actual "fallback term" class that can not be used in |
|
Ok, I would suggest to remove everything that's weird and unused, then, but I'm ok with merging it as it is, too. |
|
I suggest to remove all this too, see #54. Should we start doing this in the planed 3.0 release? |
|
Yes, if we already start cleaning up and want to do a breaking release, lets do this as a whole. |
|
Since this patch also touches methods we do not want to remove, I suggest to merge this before #54. |
Methods to add, set or remove terms and multiterms from a Fingerprint do not require a language code to be passed together with the term because the language can be derived from the term object. In the current state there can be inconsistencies in the datamodel allowing a wrong language code to be set for a term in another language.
thiemowmde
force-pushed
the
fingerprint-weirdness
branch
from
931c3a8 to
22f935f
Compare
Methods to add, set or remove terms and multiterms from a Fingerprint do
not require a language code to be passed together with the term because
the language can be derived from the term object. In the current state
there can be inconsistencies in the datamodel allowing a wrong language
code to be set for a term in another language.
Note: Tests need to be updated. This is a breaking change!
Bug: T121433