The proper implementation would be to separate the timestamp logic
(calculating how long ago, adjusting based on user timezone preferences,
etc.) and the language logic (getting messages for the day of the week,
formatting the date and time, actually making the message).
Then, have MWTimestamp process the timestamp and then pass the data onto
Language to do the formatting. That way it happens transparently.
*--*
*Tyler Romeo*
Stevens Institute of Technology, Class of 2015
Major in Computer Science
| tylerromeo(a)gmail.com
On Fri, Oct 26, 2012 at 5:33 PM, Terry Chay <tchay(a)wikimedia.org> wrote:
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 <innocentkiller(a)gmail.com> wrote:
On Thu, Oct 25, 2012 at 10:50 AM, Tyler Romeo
<tylerromeo(a)gmail.com>
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
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l