wmde / Number

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

Tweak comment in DecimalValue::newFromArray #108

Closed filbertkm closed 5 years ago

filbertkm commented 7 years ago

Using DataValueDeserializer is one alternative to this method.

manicki commented 7 years ago

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 https://github.com/DataValues/Number/issues/106.

manicki commented 7 years ago

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:

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:

thiemowmde commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

"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.

mariushoch commented 6 years ago

+1

JeroenDeDauw commented 5 years ago

https://github.com/DataValues/Number/pull/125