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.

Sign up for GitHub

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

Jump to bottom

Major rewrite of TimeFormatter #49

Closed
wants to merge 1 commit into from
Closed

Major rewrite of TimeFormatter #49

wants to merge 1 commit into from

Conversation

thiemowmde
Copy link
Contributor

@thiemowmde thiemowmde commented Mar 11, 2015 •
edited
Loading

@thiemowmde thiemowmde mentioned this pull request Mar 11, 2015
Closed
adrianheine
adrianheine reviewed Mar 16, 2015
View reviewed changes
src/ValueFormatters/TimeFormatter.php Outdated

/**
* @deprecated since 0.7, use \ValueParsers\TimeParser::CALENDAR_JULIAN instead.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage of having this in the parser instead of the formatter? How about putting it in the value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. If the constant is needed in both, these two options seem equality arbitrary to me. It's also rather odd to have the formatter depend on the parser or the other way around. Also consider that it's not actually the time formatter or the time parser, rather a time formatter and a time parser.

Unless the value uses the constants, it'd not put them in there. How about creating a dedicated enum like construct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with pretty much every solution, but this is unrelated to this patch. The two constants in TimeFormatter are not used any more in TimeFormatter with my patch. They need to be deprecated no matter what.

@thiemowmde
Copy link
Contributor Author

@adrianheine, @JeroenDeDauw: I reworked this patch completely. Now it's an actual formatter. A "basic, language-independent TimeValue to plain text formatter". The way to generic class name still needs a rename. Will do this after this is merged.

  • It's language-independent and outputs ISO-like formats as long as no option is given.
  • It omits meaningless information, e.g. leading zeros and the appended "Z".
  • It respects the given precision to a degree (not possible for precisions bigger than a year).
  • It's bulletproof and will never fail for future calendar models and timestamp representations.

@thiemowmde thiemowmde force-pushed the timeFormatterCal2 branch 3 times, most recently from a3ed93e to edaf292 Compare March 24, 2015 14:15
@thiemowmde
Copy link
Contributor Author

-1 for now. I want to split this patch into two:

  1. The main refactoring that makes this formatter aware of the precision.
  2. The interpretation of the calendar model.

Done. The actual changes in this patch are:

  • I removed the OPT_CALENDARNAMES default with the English calendar names. The formatter should be called with this option set properly, otherwise it will fall back to display URIs.
  • I made the fall back formatting (when OPT_TIME_ISO_FORMATTER is not set) aware of the precision.
  • The fact that the formatter always shows "Gregorian" in brackets is not changed in this patch.

@thiemowmde thiemowmde force-pushed the timeFormatterCal2 branch 2 times, most recently from c617841 to 94769b6 Compare April 1, 2015 10:22
@thiemowmde thiemowmde added this to the 0.7 milestone Apr 14, 2015
This was referenced Apr 14, 2015
Merged
Closed
@thiemowmde thiemowmde force-pushed the timeFormatterCal2 branch 2 times, most recently from ff8bb83 to d2ce3a8 Compare April 17, 2015 11:28
@thiemowmde thiemowmde removed this from the 0.7 milestone Apr 20, 2015
@thiemowmde thiemowmde changed the title Fix outdated calendar logic in TimeFormatter Major rewrite of TimeFormatter Apr 20, 2015
@thiemowmde thiemowmde modified the milestone: 0.8 Apr 20, 2015
@thiemowmde thiemowmde force-pushed the timeFormatterCal2 branch 3 times, most recently from 41762f1 to 9151de5 Compare June 8, 2015 09:38
@addshore
Copy link
Contributor

addshore commented Jun 8, 2015

Needs a rebase!

@thiemowmde thiemowmde removed this from the 0.8 milestone Jun 24, 2015
@thiemowmde thiemowmde mentioned this pull request Jun 24, 2015
Merged
@thiemowmde thiemowmde added this to the 0.8.2 milestone Sep 9, 2015
JonasKress
JonasKress reviewed Sep 24, 2015
View reviewed changes
src/ValueFormatters/TimeFormatter.php Outdated
$formatted = $value->getTime();
$formatted = $this->getFormattedTimestamp( $value );
// FIXME: Temporarily disabled.
//$formatted .= ' (' . $this->getFormattedCalendarModel( $value->getCalendarModel() ) . ')';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we fix this before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We removed this temporarily to have less confusing information in diffs. If we all agree what the calendar model means, and we all agree that this consensus should be visible to our users, then yes, this line should be enabled. On the other hand: This formatter is currently not used. We use the HTML formatter instead.

I would like to discuss this in an other pull request and merge this as it is. This plain text formatter is primarily meant as a fall back for external users of our libraries.

@thiemowmde thiemowmde removed this from the 0.8.2 milestone Sep 29, 2015
@thiemowmde
Copy link
Contributor Author

Why was this not merged in 0.8.2? What's so confusing about this simple enhancement that it's still sitting here after more than 6 months? I don't get that and it makes me sad and angry. What should I do? Close this and reopen it as a new pull request, where it does not have any confusing comments? Split it into so many smaller patches that people start complaining about to many untracked patches? Open tickets on Phabricator that will never be added to a sprint? What should I do to get out of the frustrating situation that finished patches that don't need any discussion get stuck forever?

@thiemowmde thiemowmde mentioned this pull request May 25, 2016
Merged
@addshore
Copy link
Contributor

Closing very old PRs

@addshore addshore closed this Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thiemowmde @addshore @adrianheine @JeroenDeDauw @JonasKress