Stop using formatter constants in parsers #54
Conversation
| @@ -30,11 +30,12 @@ protected function stringParse( $value ) { | |||
| switch ( $value ) { | |||
| case '': | |||
| case 'gregorian': | |||
| return TimeFormatter::CALENDAR_GREGORIAN; | |||
| return IsoTimestampParser::CALENDAR_GREGORIAN; | |||
There was a problem hiding this comment.
Having the constants in IsoTimestampParser seems rather arbitrary to me as well. Just saying, not objecting to this change.
There was a problem hiding this comment.
Yes, true, I was thinking the same. Will move them to TimeValue, I guess.
There was a problem hiding this comment.
As I mentioned elsewhere, I think they should not go there unless TimeValue itself uses them.
There was a problem hiding this comment.
They don't "go" anywhere, they are already there, duplicated in both "formatter" and "parser" classes. I'm trying to deprecate at least one set of these constants now. This patch here is only a small step forward. More to come, as always.
thiemowmde
force-pushed
the
formatterConsts
branch
2 times, most recently
from
cfaa61f
to
11bea50
Compare
thiemowmde
force-pushed
the
formatterConsts
branch
from
11bea50
to
e89d3cd
Compare
| new TimeValue( $timestamp, 0, 0, 0, $precision, $calendarModel ), | ||
| $expected, | ||
| $options | ||
| ); |
There was a problem hiding this comment.
Just make the arguments to the test function optional. Much more obvious than this post-processing here.
There was a problem hiding this comment.
#49 contains a major refactoring of this. I did not wanted to repeat everything, this patch here contains only very basic stuff.
|
Needs a rebase. Would merge otherwise. |
thiemowmde
force-pushed
the
formatterConsts
branch
from
e89d3cd
to
71fc45f
Compare
thiemowmde
force-pushed
the
formatterConsts
branch
from
71fc45f
to
2c0d5e2
Compare
|
Rebased. Note that this is probably obsolete via #65. |
|
I have merged #65 so will close this |
TimeParserwill be renamed toIsoTimestampParserin #39. That's why I'm usingas. This will make the following patch super-trivial.