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

Add createFromFormat static method to CarbonInterval #1921

Merged

Conversation

xfudox
Copy link

@xfudox xfudox commented Oct 29, 2019

Added createFromFormat method to CarbonInterval's core for #1918 .

Useful to handle time interval strings like MySQL time field (e.g.: '10:20:00'):
E.g.:

CarbonInterval::createFromFormat('H:i:s', '10:20:00')

is equivalent to:

list($hours, $minutes, $seconds) = explode(':', '10:20:00');
new CarbonInterval(0, 0, 0, 0, $hours, $minutes, $seconds);

or

CarbonInterval::hours($hours)->minutes($minutes)->seconds($seconds);

@kylekatarnls kylekatarnls added this to the 2.26 milestone Oct 29, 2019
@kylekatarnls
Copy link
Collaborator

Hi @xfudox, thanks for your PR, would you mind please adding unit tests for your feature.

See as an example:
make test:
https://github.com/briannesbitt/Carbon/blob/master/tests/CarbonInterval/ConstructTest.php#L283
or __construct test:
https://github.com/briannesbitt/Carbon/blob/master/tests/CarbonInterval/ConstructTest.php#L21

Thanks

@kylekatarnls kylekatarnls changed the title added new createFromFormat method to CarbonInterval Added createFromFormat static method to CarbonInterval Oct 29, 2019
Copy link
Collaborator

@kylekatarnls kylekatarnls left a comment

Choose a reason for hiding this comment

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

  • Unit tests
  • Fix PHPDoc

src/Carbon/CarbonInterval.php Outdated Show resolved Hide resolved
@kylekatarnls
Copy link
Collaborator

Hi @xfudox I had no time to fix the PHPDoc and add unit tests, so I postponed this for version 2.27. Thanks for your understanding.

@xfudox
Copy link
Author

xfudox commented Nov 5, 2019

Yeah, sorry for I'm late, I've very little free time to work on this recently, hope to fix this asap.

@kylekatarnls
Copy link
Collaborator

Hi @xfudox thanks for unit tests and doc. The unit tests are not passing in Travis. Can you have a look?

@xfudox
Copy link
Author

xfudox commented Nov 8, 2019

Yeah @kylekatarnls , I'll take a look asap.

@kylekatarnls kylekatarnls assigned kylekatarnls and unassigned xfudox Nov 14, 2019
@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Nov 14, 2019

Obviously, my solution (the macro proposition made in the issue) will only work with hours, not for day/month/year, so we actually need to parse the format in a custom way. I take the assignee to propose something else.

@kylekatarnls kylekatarnls dismissed their stale review November 14, 2019 16:02

Changes done myself

@kylekatarnls
Copy link
Collaborator

Hi @xfudox can you check the new implementation?

@kylekatarnls kylekatarnls assigned xfudox and unassigned kylekatarnls Nov 14, 2019
@kylekatarnls kylekatarnls changed the title Added createFromFormat static method to CarbonInterval Add createFromFormat static method to CarbonInterval Nov 17, 2019
@kylekatarnls kylekatarnls merged commit 630c8c8 into briannesbitt:master Nov 19, 2019
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.

3 participants