General update #16
General update #16
Conversation
|
Did only a rough review, though this looks like it's going in the right direction and does not contain any serious issues design wise |
snaterlicious
force-pushed
the
fingerprint
branch
2 times, most recently
from
7cb4aca
to
ac83724
Compare
|
What did you use as reference when implementing this? Some documentation, spec, code? |
Added qualifier tests and slimmed down existing tests to what is useful.
Added constructor tests and slimmed down .equals() test to what is actually useful.
| */ | ||
| var SELF = wb.datamodel.Statement = util.inherit( 'WbStatement', PARENT, constructor, { | ||
| var SELF = wb.datamodel.Statement = function WbDataModelStatement( claim, references, rank ) { |
There was a problem hiding this comment.
Just wondering. The constructor in WikibaseDataModel is missing the third rank parameter. Should we add it there?
|
The following fails: deserializer.deserialize( {
type: 'item',
id: 'Q1',
labels: {
"be-tarask":{"language":"be-tarask","value":"a"},
"be-x-old":{"language":"be-tarask","value":"b"}
}
} );Error message is |
|
|
|
We could also work around this in wikibase.git instead of here. |
|
I could imagine us running into trouble if we try to work around that in Wikibase.git. Especially, since I do not see a chance of the issue being resolved in the near future. Although I do not like the idea at all either, still, it might be the cleanest solution to (re-)implement a LegacyEntityId and the corresponding Serializer/Deserializer in the JS serialization pull request. If there are no objections, I will amend the pull request. |
|
Works for me, too. |
|
I think the LegacyEntityId has to be used for |
|
|
||
| for( var i = 0; i < this._items.length; i++ ) { | ||
| var propertyId = this._items[i].getMainSnak().getPropertyId(); | ||
| if( $.inArray( propertyId, propertyIds ) === -1 ) { |
There was a problem hiding this comment.
An inArray check in a loop is not so nice perfomance-wise. Could be refactored using a map instead. But not a reason to not merge this patch.
There was a problem hiding this comment.
We could just use jQuery.map and jQuery.unique. Probably faster.
There was a problem hiding this comment.
Would be nice to actually get data on that. I remember seeing some website where you could easily compare such things - can't find it now though
There was a problem hiding this comment.
We can write our own test on http://jsperf.com/.
There was a problem hiding this comment.
jQuery.unique works on DOM elements only.
There was a problem hiding this comment.
Oh, right. _.uniq would work, but we don't have underscore.
|
+1. I'm fine with merging this as it is. There are some nested loops I don't like for performance reasons. But these can be easily refactored in later patches. |
Needs * wmde/WikibaseDataModelJavaScript#16 * wmde/WikibaseSerializationJavaScript#6 Bug: 72757 Change-Id: Iafc2ed96df9f4a36b87ea9217eddc653fd3c4b9c
Updated component in compliance with DataModel 1.0 aiming at being able to properly manage Fingerprint, Item and Property objects in front-end code. Functionality supposed to be obsolete (e.g. toJSON(), newFromJSON()) is removed in favour of using the wikibase.serialization component (see wmde/WikibaseSerializationJavaScript#6 for corresponding changes to that component).
The changes are supposed to add completeness to the component being able to properly model the UI using objects provided by the component. Inspired by the back-end implementation of the data model, there are, however, a few differences. Most obvious is the more intuitive and consistent naming of constructors ending with
_Set,_Listand*Group. The concept of groups is not a dedicated part of the back-end data model and is, instead, applied and resolved during serialization/deserialization. However, since grouping is an essential logical concept, it should be an integral part of the JS data model.