Skip to content

Implement diffForHumans#57

Merged
lorenzo merged 8 commits intomasterfrom
diff-for-humans
Nov 22, 2015
Merged

Implement diffForHumans#57
lorenzo merged 8 commits intomasterfrom
diff-for-humans

Conversation

@markstory
Copy link
Copy Markdown
Member

This shores up a big incompatibility between Carbon and Chronos. I've only implemented english, as I feel that is the minimal set of messages we need for compatibility. I also just didn't want to add any dependencies to chronos.

Refs cakephp/cakephp#7713

This current implementation is only handling english. The Translator
class is fairly primitive but gives the extension point we'll need later
on.
@markstory markstory added this to the 0.5.0 milestone Nov 19, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ param ...|null missing

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks.

@HavokInspiration
Copy link
Copy Markdown
Member

This is looking good.
I fixed a bunch of CS warnings.

How do you suggest we provide other languages than english ?

@lorenzo
Copy link
Copy Markdown
Member

lorenzo commented Nov 20, 2015

I was more inclined to see the implementation of the diff inside another object. Do you think that would make sense?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't it be diffForHumans ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should.
I would also uppercase the removes: bool $absolute Removes

@markstory
Copy link
Copy Markdown
Member Author

@lorenzo Ok. That might make it easier to integrate CakePHP's i18n subsystem.

Having a separate object for the difference formatter will let us rip it
out and replace it in CakePHP with a more complete i18n approach.
@markstory
Copy link
Copy Markdown
Member Author

@lorenzo I've split out the formatter, let me know what you think.

@lorenzo
Copy link
Copy Markdown
Member

lorenzo commented Nov 22, 2015

Good job @markstory!

lorenzo added a commit that referenced this pull request Nov 22, 2015
@lorenzo lorenzo merged commit 4b0d374 into master Nov 22, 2015
@lorenzo lorenzo deleted the diff-for-humans branch November 22, 2015 13:46
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

Successfully merging this pull request may close these issues.

4 participants