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

Allows createFromTime('00:00:00') #919

Closed
rentalhost opened this issue Mar 23, 2017 · 4 comments
Closed

Allows createFromTime('00:00:00') #919

rentalhost opened this issue Mar 23, 2017 · 4 comments
Assignees
Milestone

Comments

@rentalhost
Copy link
Contributor

Currently the createFromTime() method expects four arguments: hour?, minute?, second? and tz?. Should be great if it could support a string that defines hours and minutes (and optionally seconds). It will help if you are loading data from SQL, for instance:

$time = Carbon::createFromTime('02:40');    // 02 hours and 40 minutes
$time = Carbon::createFromTime('02:40:35'); // 02 hours, 40 minutes and 35 seconds

I could make a PR if you think that it's valid.

@Glavic
Copy link
Collaborator

Glavic commented Feb 16, 2018

Sorry for late reply.

Yes, PR would be welcomed. Maybe name of method should be createFromTimeString ?

@Glavic
Copy link
Collaborator

Glavic commented Mar 4, 2018

$time = Carbon::createFromTime('02:40'); // 02 hours and 40 minutes

To me this would mean 2 min 40 sec not hours and minutes..

@kylekatarnls
Copy link
Collaborator

Hi, I would prefer one method for each data input type. And as we have setTime and setTimeFromTimeString (see: http://carbon.nesbot.com/docs/#api-settersfluent), it would be consistent to have createFromTimeString and keep setTimeFromTimeString and createFromTimeString synchronized on possible input and interpretation of it.

@Glavic currently setTimeFromTimeString('02:40') will set 2 hours 40 minutes 0 seconds so I agree for the same behavior for createFromTimeString.

@kylekatarnls kylekatarnls self-assigned this Mar 9, 2018
@kylekatarnls kylekatarnls added this to the 1.25 milestone Mar 9, 2018
kylekatarnls added a commit to kylekatarnls/Carbon that referenced this issue Mar 13, 2018
kylekatarnls added a commit to kylekatarnls/Carbon that referenced this issue Mar 13, 2018
kylekatarnls added a commit to kylekatarnls/Carbon that referenced this issue Mar 13, 2018
@kylekatarnls
Copy link
Collaborator

I close this, we can continue to discuss about this on the implementation PR #1192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants