Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Singular DateTime::[get|set]Microsecond & no tentative return type #13486

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented Feb 24, 2024

Renamed DateTime[Immutable]::[get|set]Microseconds() to DateTime[Immutable]::[get|set]Microsecond()

  • This is a getter/setter of the microsecond part of the time and not a multiple microseconds
  • If you think of hour this gets more clear as it would be [get|set]Hour and not hours

Removed tentative return type as noted here #12557 (comment)

Added getMicrosecond to DateTimeInterface as discussed in #13491

As this hasn't been added to any stable/beta branch it's not a BC break.

Copy link
Member

@Ayesh Ayesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to run php build/gen_stub.php, so the tests will eventually fail.

As for the method renaming, I'm personally against using microsecond. JS also follows the plural naming pattern (Date.setMilliseconds, Date.setHours, etc). We don't have DateTime::setHour method in the first place, so there is no inconsistency.

@marc-mabe
Copy link
Contributor Author

@Ayesh This is about one part of the date/time. As that it's about one single microsecond part and does not get/set multiple microseconds.

About JS Date object is nothing I would look at as it's a nightmare in method naming. it's a mixture of singular/plural accessors (getYear vs. getSeconds etc.) - oh you actually have to use getFullYear, a date is a day getDate and a day is day of week getDay and so on.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really care about the naming but maybe @derickr has opinions?

@kocsismate
Copy link
Member

kocsismate commented Feb 25, 2024

IMO ext/date is already inconsistent with the naming: on one hand, there is already a setMilliseconds() method, but on the other hand, we have a $microsecond parameter in the setTime() method (and all the other parameter names are singular).

BTW: the tentative return type related parts are fine, and thanks for dealing with this!

@marc-mabe
Copy link
Contributor Author

IMO ext/date is already inconsistent with the naming: on one hand, there is already a setMilliseconds() method, but on the other hand, we have a $microsecond parameter in the setTime() method (and all the other parameter names are singular).

Where is setMilliseconds defined?
I can only find it in Carbon in both singular/plural variants : https://github.com/briannesbitt/Carbon/blob/master/src/Carbon/Carbon.php#L339-L340

@kocsismate
Copy link
Member

Where is setMilliseconds defined?

Oh... I guess I mixed it up with setMicroseconds... 🤔 Because I cannot find it either.. In which case the renaming makes sense indeed!

@derickr
Copy link
Member

derickr commented Feb 27, 2024

After my other merge, this has now a conflict in the generated stub. Can you fix that? I'll then merge this.

@marc-mabe marc-mabe force-pushed the DateTime-getSetMicrosecond branch 2 times, most recently from a9e4c26 to 4898a5f Compare February 28, 2024 07:39
@marc-mabe
Copy link
Contributor Author

After my other merge, this has now a conflict in the generated stub. Can you fix that? I'll then merge this.

@derickr done rebased

@marc-mabe marc-mabe force-pushed the DateTime-getSetMicrosecond branch from 4898a5f to 8b965ef Compare March 6, 2024 08:16
@marc-mabe
Copy link
Contributor Author

@derickr @kocsismate Now added getMicrosecond to DateTimeInterface as well as discussed in #13491

@derickr derickr merged commit dbd976a into php:master Mar 6, 2024
7 of 9 checks passed
@marc-mabe marc-mabe deleted the DateTime-getSetMicrosecond branch March 6, 2024 11:29
fabpot added a commit to symfony/symfony that referenced this pull request Mar 9, 2024
…nd() (xabbuh)

This PR was merged into the 7.1 branch.

Discussion
----------

[Clock] rename get/setMicroseconds() to get/setMicrosecond()

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

follows php/php-src#13486

Commits
-------

ff06ba4 rename get/setMicroseconds() to get/setMicrosecond()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants