I approved the reversion of the patch since I was the original person
who approved it.
As I see it there are the following problems with the change I approved:
1. Typo in weekday cutoff [Timo]
2. iAlex points out that the optional parameter fix variable ordering is wrong
[Harshar]
3. need to integrate https://gerrit.wikimedia.org/r/#/c/29927/ [Raimond]
^^^ the above is trivial to fix, and should be.
4. Have to pass in a User object [Harshar].
^^^^ I feel this is resolved. The reasoning was discussed over the months this
was held in code review. The concession to these complaints was Andrew rewrote
this to be a required parameter to make the dependency explicit which
introduced Issue 2 above. This is moving goal post at this point.
4. massive discussion of MWTimestamp class which appeared (August) since after
this patch was submitted (in June)
4a. side discussion on MWTimeStamp::getHumanTimeStamp() is unlocalizeable
[Petr] (and needs to be deprecated [Niklas])
4b. prettyTimestamp() is "confusing" [Harshar]
4c. proposal to use MWTimestamp inside Language::prettyTimestamp()
5. proposal to use DateInterval [Harshar]
Now it seems to me that MWTimestamp::getHumanTimestamp() and the patch
Language::pretyTimestamp() serve the same purpose, so we need a resolution
here, because this stupid fix has been blocking Echo for a while.
The issue is simply that human readable/formatted timestamps will need
both language context and timestamp/timezone context. One of those is going to
have to win out here, I'm leaning to deprecating
MWTImestamp::getHumanTimestamp() and using Language::prettyTimestamp() because
of i18n issues.
(As for 5, that just makes the code easier to read. So I think this is
orthogonal to the discussion. It's probably a good idea to look into it now
that we're on PHP 5.3.)
On Oct 25, 2012, at 7:58 AM, Chad <[email protected]> wrote:
> On Thu, Oct 25, 2012 at 10:50 AM, Tyler Romeo <[email protected]> wrote:
>> To be clear, I am not saying that any language functionality should be in
>> MWTimestamp, because that's not what it's for. On the other side of it,
>> there should be no timestamp functionality in the Language class either. We
>> need a fix so that if MWTimestamp::getHumanTimestamp is called, Language is
>> used to generate it properly.
>
> I disagree. The human/formatting stuff absolutely should be in Language. The
> problem with having it in MWTimestamp is it requires a language or context to
> be passed to do it right. This introduces a new dependency there that I don't
> really like (MWTimestamp is nice and self-contained right now).
>
> -Chad
>
> _______________________________________________
> Wikitech-l mailing list
> [email protected]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
