Tweak comment in DecimalValue::newFromArray #108

filbertkm · 2017-06-26T12:42:15Z

Using DataValueDeserializer is one alternative to this method.

manicki · 2017-06-26T12:47:45Z

I like it! Stated that way it is less confusing (as using DataValueDeserializer is not given as the only option), which I was complaining about in #106.

thiemowmde · 2017-06-26T13:40:57Z

src/DataValues/DecimalValue.php

@@ -355,10 +355,11 @@ public function getArrayValue() {
 	 *
 	 * @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.
+	 *  newFromArrayValue). Instead, use the constructor or use DataValue builder
+	 *  callbacks. (e.g. @see DataValueDeserializer)


I find the "or" problematic because the possibility to pass callbacks to DataValueDeserializer is not an alternative for having constructors. The callback must still construct a DataValue object, and needs to call a constructor to do this. Also I find the typography (brackets outside of the sentence) weird.

Instead, use the default constructor or introduce a new static constructor with a better name, e.g. when needed for @see DataValueDeserializer.

Also I find the typography (brackets outside of the sentence) weird.

Oh, come on. Just move the full stop to the end:

Instead, use the constructor or use DataValue builder callbacks (e.g. @see DataValueDeserializer).

What I find better in the version @filbertkm is that it is more generic. What you suggest instead is:

assuming one must use some static constructor in those deserializer callbacks. Maybe it is true in the end. But far too detailed for this particular place IMO.

IMO saying "introduce a new X but call it Y when you need it" is a bit odd thing to say in the deprecation notice about removing "X".

Use of DataValueDeserializers (builders) is one alternative to this deprecated method but this component can be used on it's own. The alternative w/o use of separate component is just construct the object directly

move the full stop to the end […]

Sure, that's what I'm asking for. Why do I get a "come on" for requesting this?

assuming one must use some static constructor in those deserializer callbacks.

I don't get where I'm saying this. What I say is "Instead, use the default constructor or […]". There is an or. In most cases the default constructor will be enough.

saying "introduce a new X […] in the deprecation notice about removing "X".

I don't get where I say this either. Again: a static constructor might be convenient, but it should not be called "newFromArray". Reasons for this are explained extensively in #106.

@aude, I know that. Is this meant as a response to something @manicki or I said?

manicki · 2017-06-27T07:59:16Z

Okay, added commit moving the period to the end of the sentence.
I believe we would find better use of our time than debating on punctuation or on what this or that way of phrasing things could be interpreted right.

Here is my review for this pull request:

the new message mentions the alternative of regular constructors. It might be stating the obvious but I think it is good to mention this. The existing comment does not do it, so this is good change IMO.
the new message expresses what the existing comment also does in a way that in my personal opinion is more clear and precise.
therefore I'm OK to merge this PR and adjust related messages.

I really do not intend to spend days on crafting the best depreciation comment message ever. But as the new comment was suggested, and it is in my opinion better than the one there is now, I think it is good to change it. My take on this PR would be that either:

there is a reason brought up why the suggested text is wrong (i.e. if it included information which is incorrect or misleading for users) - this is not a case as far as I can tell.
we accept the new comment message as suggested by @filbertkm ,
or there is a better alternative comment suggested which we like the best (the alternative suggested above by @thiemowmde is in my opinion not that good as what @filbertkm proposes). If there are other ideas, now it is a time to bring them up here!

thiemowmde · 2017-06-27T09:02:01Z

I don't like to repeat myself, but it appears what I said was not heard: Stating "use the constructor or use DataValue builder callbacks" sounds like callbacks can act as replacements for constructors, which is not possible.

Please tell me what's wrong with Instead, use the default constructor or introduce a new static constructor with a better name, e.g. when needed for @see DataValueDeserializer.

manicki · 2017-06-27T09:27:39Z

Okay, as you asked.

Please tell me what's wrong with Instead, use the default constructor or introduce a new static constructor with a better name, e.g. when needed for @see DataValueDeserializer.

To me the second part of this sentence ("introduce a new static constructor with a better name, e.g. when needed") seems a really surprising thing to be said in the depreciation notice. Isn't what you're saying (in the second part, I know you mention the regular constructor in the first part) basically: "Don't use this. Instead introduce something new if you need. Just give it a good name"?

Reading it now, I would not have called __construct "default constructor" either. This is the only constructor exists in PHP, isn't it? But this is a minor detail.

thiemowmde · 2017-06-27T10:00:42Z

"Static constructor methods", short "static constructors". "Default constructor" seems to be much more ambiguous, but what else is __construct?

Isn't what you're saying […] basically: […]

Yes, that's it. It appears you think there is something wrong with this, but I don't get it.

thiemowmde

I simplified the comment a lot, because the class in question here really does not need a more convenient way to be constructed. If this is fine for you I will update similar comments in other classes.

mariushoch · 2018-03-16T17:14:50Z

+1

JeroenDeDauw

The migration path away from newFromArray is DataValueDeserializer, so removing the mention of it is not good.

To replace #108

JeroenDeDauw · 2019-01-04T19:53:29Z

#125

Tweak comment in DecimalValue::newFromArray

5f128be

Using DataValueDeserializer is one alternative to this method.

filbertkm requested review from manicki and thiemowmde June 26, 2017 12:42

filbertkm mentioned this pull request Jun 26, 2017

Better documentation for the DataValue::newFromArray contract #106

Merged

manicki approved these changes Jun 26, 2017

View reviewed changes

thiemowmde suggested changes Jun 26, 2017

View reviewed changes

Fixed punctuation

9d5e7c0

Update DecimalValue.php

c9ca43e

thiemowmde approved these changes Jun 30, 2017

View reviewed changes

JeroenDeDauw suggested changes Aug 2, 2018

View reviewed changes

JeroenDeDauw added a commit that referenced this pull request Jan 4, 2019

Tweak comment in DecimalValue::newFromArray

44119cc

To replace #108

JeroenDeDauw mentioned this pull request Jan 4, 2019

Tweak comment in DecimalValue::newFromArray #125

Open

JeroenDeDauw closed this Jan 4, 2019

JeroenDeDauw deleted the decimalvalue-newfromarray branch January 4, 2019 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak comment in DecimalValue::newFromArray #108

Tweak comment in DecimalValue::newFromArray #108

filbertkm commented Jun 26, 2017

manicki commented Jun 26, 2017

thiemowmde Jun 26, 2017 •

edited

Loading

manicki Jun 26, 2017

filbertkm Jun 26, 2017

thiemowmde Jun 26, 2017 •

edited

Loading

thiemowmde Jun 26, 2017

manicki commented Jun 27, 2017

thiemowmde commented Jun 27, 2017

manicki commented Jun 27, 2017

thiemowmde commented Jun 27, 2017 •

edited

Loading

thiemowmde left a comment

mariushoch commented Mar 16, 2018

JeroenDeDauw left a comment

JeroenDeDauw commented Jan 4, 2019

Tweak comment in DecimalValue::newFromArray #108

Tweak comment in DecimalValue::newFromArray #108

Conversation

filbertkm commented Jun 26, 2017

manicki commented Jun 26, 2017

thiemowmde Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

manicki Jun 26, 2017

Choose a reason for hiding this comment

filbertkm Jun 26, 2017

Choose a reason for hiding this comment

thiemowmde Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

thiemowmde Jun 26, 2017

Choose a reason for hiding this comment

manicki commented Jun 27, 2017

thiemowmde commented Jun 27, 2017

manicki commented Jun 27, 2017

thiemowmde commented Jun 27, 2017 • edited Loading

thiemowmde left a comment

Choose a reason for hiding this comment

mariushoch commented Mar 16, 2018

JeroenDeDauw left a comment

Choose a reason for hiding this comment

JeroenDeDauw commented Jan 4, 2019

thiemowmde Jun 26, 2017 •

edited

Loading

thiemowmde Jun 26, 2017 •

edited

Loading

thiemowmde commented Jun 27, 2017 •

edited

Loading