On Thu, Oct 25, 2012 at 10:16 AM, Antoine Musso hashar+wmf@free.fr wrote:
I would prefer we revert that change and improve it, possibly using PHP DateInterval and definitely with MWTimestamp.
Created a revert patch set in https://gerrit.wikimedia.org/r/#/c/29947/
Siebrand
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. *--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Thu, Oct 25, 2012 at 6:27 AM, Siebrand Mazeland s.mazeland@xs4all.nlwrote:
On Thu, Oct 25, 2012 at 10:16 AM, Antoine Musso hashar+wmf@free.fr wrote:
I would prefer we revert that change and improve it, possibly using PHP DateInterval and definitely with MWTimestamp.
Created a revert patch set in https://gerrit.wikimedia.org/r/#/c/29947/
Siebrand
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Thu, Oct 25, 2012 at 10:50 AM, Tyler Romeo tylerromeo@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
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@gmail.com wrote:
On Thu, Oct 25, 2012 at 10:50 AM, Tyler Romeo tylerromeo@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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 www.whizkidztech.com | tylerromeo@gmail.com
On Fri, Oct 26, 2012 at 5:33 PM, Terry Chay tchay@wikimedia.org wrote:
I approved the reversion of the patch since I was the originalperson who approved it.
As I see it there are the following problems with the change Iapproved:
- Typo in weekday cutoff [Timo]
- 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.
- 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.
- 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 thepatch 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 willneed 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 thinkthis 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@gmail.com wrote:
On Thu, Oct 25, 2012 at 10:50 AM, Tyler Romeo tylerromeo@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Fri, Oct 26, 2012 at 6:07 PM, Tyler Romeo tylerromeo@gmail.com wrote:
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.
If you're passing the output of MWTimestamp to Language, it's not transparent. If you want to do it transparently, you have to keep the accessor in MWTimestamp and pass a Language object (which I've already argued against).
I see no harm in using MWTimestamp in Language, but please let's not do it the other way around.
-Chad
So basically instead of passing Language to MWTimestamp you want to pass MWTimestamp to Language. IMHO it's fine with me, so long as the logic is properly separated. In the end, all this would require is the addition of MWTimestamp::diff (or since) and MWTimstamp::userAdjust, which Language would call in order to get the proper timestamps.
I can work on a patch for this now. *--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Fri, Oct 26, 2012 at 6:18 PM, Chad innocentkiller@gmail.com wrote:
On Fri, Oct 26, 2012 at 6:07 PM, Tyler Romeo tylerromeo@gmail.com wrote:
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.
If you're passing the output of MWTimestamp to Language, it's not transparent. If you want to do it transparently, you have to keep the accessor in MWTimestamp and pass a Language object (which I've already argued against).
I see no harm in using MWTimestamp in Language, but please let's not do it the other way around.
-Chad
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
wikitech-l@lists.wikimedia.org