nette / utils

🛠 Lightweight utilities for string & array manipulation, image handling, safe JSON encoding/decoding, validation, slug or strong password generating etc.
https://doc.nette.org/utils
Other
1.98k stars 147 forks source link

DateTime: method setTime() allow null value for keep current value #301

Closed h4kuna closed 1 year ago

h4kuna commented 1 year ago

I keep default values like a parent. The back compatibility is ok.

You can more simply modify part of time. For example you want reset microseconds

dump((new Nette\Utils\DateTime())->applyTime(null, null, null, 0));
dump((new Nette\Utils\DateTime())->applyTime(microsecond: 0));

More examples are in test.

dg commented 1 year ago

This is pretty tricky. To make it sensible, setTime(microsecond: 0) would have to work, because the notation with many zeros and null is not good. But if you implement it that way, the whole thing can behave in surprising ways.

h4kuna commented 1 year ago

Yes you have right, all parameters must have default value null. The example setTime(microsecond: 0) will work right. But I keep back compatibility. Because if all parameters will be null, this exists code will to work bad. setTime(1, 1).

The way where are all parameters null I like it more too.

What do you suggest?

  1. remove all default values?
  2. one same value for all parameters? null / 0
  3. change method name? setTime() to applyTime(), withTime(), useTime()
  4. anything else?
dg commented 1 year ago

I suggest to drop it. It's a little needed functionality, it smells like a problem and I would be the one who would have to deal with it :-)

h4kuna commented 1 year ago

ok