search

wmde / Number

Numerical value objects, parsers and formatters
Other
18 stars 6 forks source link

Better documentation for the DataValue::newFromArray contract #106

Closed thiemowmde closed 7 years ago

thiemowmde commented 7 years ago

105 already fixed it, this is just making the contract much more clear, I hope.

Bug: T168681

thiemowmde commented 7 years ago

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 array type hints. How complicated could that be? It seems it can, even to a point where everything just gets stuck. Why? What are you missing? What did I do wrong? Do you think this is not relevant any more? Did I wasted my time? Who will continue working on this, if what I did is not sufficient?

manicki commented 7 years ago

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.

thiemowmde commented 7 years ago

@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 @todo somewhere in DataValueDeserializer or a Phabricator ticket can't do this.

manicki commented 7 years ago

hmm, then if in principle it would be OK to keep static newFromArray constructors (possible having them renamed), why deprecate them?

thiemowmde commented 7 years ago

[…] 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.