Implement UnDeserializableValue::newFromJSON and toJSON #78
Conversation
filbertkm
force-pushed
the
undeserializable
branch
from
0aaa26f
to
b2429da
Compare
| function( ofType, unUnserializableStructure, unserializeError ) { | ||
| if( !$.isPlainObject( unUnserializableStructure ) ) { | ||
| throw new Error( 'The un-unserializable structure has to be a plain object' ); | ||
| function( ofType, unDeserializableStructure, deserializeError ) { |
There was a problem hiding this comment.
The PHP class uses the order $data, $type, $error. Not sure what's better and if it's worth the trouble to change one of the implementations. What do you think?
|
If you want we can merge this and fix the tiny issues later. Please tell me. |
|
I think we need to go back to the way I ordered the params before. UnDeserializableValue inherits from dv.DataValue which has a specific order. I think it's best to be consistent with that, even if it's different that in php. |
filbertkm
force-pushed
the
undeserializable
branch
2 times, most recently
from
01390fe
to
867ee33
Compare
|
What do you mean with "specific order [in] dv.DataValue"? |
| return { | ||
| type: 'undeserializable', | ||
| data: { | ||
| targetType: this.getTargetType(), |
There was a problem hiding this comment.
This is quite different from how it was before. I do not know how much this matters. What was your reason to change it?
filbertkm
force-pushed
the
undeserializable
branch
from
867ee33
to
122b1d3
Compare
|
|
||
| /** | ||
| * @inheritdoc | ||
| * | ||
| * @return {string} | ||
| * @return {int} |
There was a problem hiding this comment.
Why int? This is the error message, which is a string, right?
|
@thiemowmde suppose the order doesn't totally matter, but in SnakDeserializer we have: this.newDataValue = function( dataValueType, data ) it is independent though from the order we order the params here, and suppose we could change. i just prefer the order to be same as there. |
|
For reference:
|
filbertkm
force-pushed
the
undeserializable
branch
from
122b1d3
to
b6d6727
Compare
made the error attribute of the value be a string, with the error reason, for more consistency with the backend implementation and for better ease of use. also fixed more of the wording and variable names from "ununserializable" to "undeserializable", for consistency purposes. Bug: T99231
filbertkm
force-pushed
the
undeserializable
branch
from
b6d6727
to
d1719be
Compare
|
gone back again and made the order "value, type, error" throughout the value class and am adjusting the SnakSerializer. ( i really don't care, but suppose it's consistent with php is a good reason) |
|
and squashed the commits |
|
Thanks. +2 when the tests succeed. |
|
https://phabricator.wikimedia.org/T96892 is the correct task |
Implement UnDeserializableValue::newFromJSON and toJSON
made the error attribute of the value be a string, with
the error reason, for more consistency with the backend
implementation and for better ease of use.
also fixed more of the wording and variable names from
"ununserializable" to "undeserializable", for consistency purposes.
Bug: T99231