Better documentation for the DataValue::newFromArray contract #106
Conversation
| * | ||
| * @deprecated since 0.8.3. Static DataValue::newFromArray constructors like this are | ||
| * underspecified (not in the DataValue interface), and misleadingly named (should be named | ||
| * newFromArrayValue). Instead, use DataValue builder callbacks in @see DataValueDeserializer. |
There was a problem hiding this comment.
hmm, I am not sure if this is framed the best way. DataValueDeserializer indeed makes use of those constructors. But why would this library bother about some DataValueDeserializer? And, probably not in this case, in theory, there could be other users of those constructors than DataValueDeserializer.
Also, I note that builder callback for "wikibase-entityid" in Wikibase does use EntityIdValue::newFromArray :)
I know it is good to provide alternatives when deprecating a part of public interface, but maybe simple "DO NOT USE" would be enough here?
There was a problem hiding this comment.
The only reason the newFromArray methods exist is because DataValueDeserializer needs them. DataValue::getArrayValue serializes, newFromArray deserializes. That was the idea.
I don't care much about other code. What's relevant and really, really crucial here is to know how this is going to be used in DataValueDeserializer. I tried to add the contract of "newFromArray" including this big comment to the DataValue interface, but I figured this would make the situation worse. We already have an implementation without a "newFromArray", and we should be free to remove them individually when we are sure they are not used any more. That's why it should not be in the interface, and that's why the deprecation must mention the alternative.
There was a problem hiding this comment.
Sure, I understand that. But as I pointed out, using "builder callback" does not mean it would not call newFromArray (which it does now).
From the perspective of this component this depreciation means: do not use newFromArray, use regular constructor, or do I get it wrong?
There was a problem hiding this comment.
I want to get rid of the bad code in DataValueDeserializer that just assumes every DataValue implementation does have a newFromArray method with a specific contract, because this contract is not specified anywhere.
The deprecation is mostly because of the bad name. Builder callbacks are still free to use whatever constructor they want (the default one or a more convenient static one).
There was a problem hiding this comment.
I understand that. I do not oppose to depreciation or anything. I am only mentioning that I found the depreciation notice a bit confusing (a single sentence there, in particular).
I don't know this all DataValues stuff well, so it would be the best if @JeroenDeDauw or @brightbyte chimed in here I guess. It simply seems a bit odd to me that this components mentions some other component which is not even its dependency etc.
There was a problem hiding this comment.
Such much comments... read the first few
DataValueDeserializer was added to allow getting rid of these static construction methods. Apparently no deprecation tag was added to these specific classes. Doing so should not be an issue. Perhaps removing them altogether also is not? If migration to DataValueDeserializer has been completed, you can just update the code in DataValueDeserializer and ditch these methods. About time after 4(?) years.
I agree that generally mentioning a class from a component you do not even depend on is either weird or flat out bad. Since the responsibility for this serialization and deserilaization stuff was moved to another component, it does seem like the sensible thing to do here. And anyway, no need to spend lots of time on writing the best comment ever if you just get rid of it and the code it goes along with in the near future (which is what I hope and suggest you do).
There was a problem hiding this comment.
DataValueDeserializer exists since 2013. Not only was it the only caller of the DataValue::newFromArray methods. The call in DataValueDeserializer was actually the specification of how all DataValue::newFromArray methods must behave. Which is why I started calling this a "circular dependency" in a comment above. An alternative was added in 2016 with DataValues/Serialization#14.
There was a problem hiding this comment.
Not only was it the only caller of the DataValue::newFromArray methods. The call in DataValueDeserializer was actually the specification of how all DataValue::newFromArray methods must behave.
Wrong. As I wrote, newFromArray predates DataValueDeserializer, the later being added to migrate away from the former.
Which is why I started calling this a "circular dependency" in a comment above.
That is poor use of words IMO, since this most certainly is not a circular dependency. You can broaden the term sure, but if you go this far, it becomes so vague that it loses its utility.
An alternative was added in 2016 with DataValues/Serialization#14.
Yeah cool. This is the kind of thing that was always meant to happen to finish the migration.
This is just talking about the past. I'd rather focus on how to proceed now. Anyway, going to ubsub here and leave this in your (collective) capable hands.
There was a problem hiding this comment.
Wrong.
It appears you did not read what I wrote, or just don't want to understand what I meant. I find this really frustrating. Again: since 2013 the detailed specification of how all newFromArray methods must behave was not in the DataValue interface but in DataValueDeserializer. How do you call this, if not a circular dependency?
There was a problem hiding this comment.
@thiemowmde I don't really care too much about the wording but #108 is what I had/have in mind. Using builders in DataValueSerializer is one alternative.
Hopefully we can remove this method entirely at some point soon
|
It's 7 hours since I uploaded this patch. Then we spend almost 2 hours discussing what I did, without getting anywhere. Then everybody stopped. This makes me angry and helpless. I spend the entire morning on T168681 and all related patches. I was hoping we could do the bugfix releases today. Really, in essence they do nothing but removing two |
|
Thanks @JeroenDeDauw for explaining. With this information it was clear to me this should be deprecated. When this PR was opened, it has probably been the second time I ever looked in any of this data values code. That's why I wanted someone else to confirm. Now with all the comments, and looking some more, I can see what is the intended direction here. But really, it does not strike as obvious that newFromArray should have been deprecated and that this DataValueDeserializer is the replacement for newFromArray, when both DataValueDeserializer and its famous callbacks happily use newFromArray. |
|
@manicki, technically, there is nothing wrong with having static constructors. Each DataValue implementation is entirely free to have one or more. The main problem we are talking about here is neither caused by the pure existence of the newFromArray methods, nor by the callbacks using them, but solely by the fact that DataValueDeserializer expects them to exist and behave in a very specific way that is not specified in the DataValue interface. If this would be the only problem with the newFromArray methods, it would not be necessary to deprecate them. But there are more problems, as I already tried to explain above. The name is misleading as not all of them expect an array. In simple cases like StringValue the method does not do anything but calling the default constructor. Also the deprecation makes all the problems we are talking about here much, much more obvious. A |
|
hmm, then if in principle it would be OK to keep static newFromArray constructors (possible having them renamed), why deprecate them? |
Because the name is bad and new ones with better names should be introduced, if needed. Oh yea, and because I believe a deprecation phase is worth it. A rename would be a breaking change. |
#105 already fixed it, this is just making the contract much more clear, I hope.
Bug: T168681