Forum: IronRuby Code Review - Time#strftime ignore invalid directives on format string

B3068fb9bae93b2889ffc73feb1b3195?d=identicon&s=25 Enrico Sada (Guest)
on 2011-08-02 19:07
(Received via mailing list)
i asked a pull request ( https://github.com/IronLanguages/main/pull/28
) for fix Time#strftime behaviour on invalid directives on format
string, ex:

ruby 1.8:
Time.now.strftime '%$' => '$'
ruby 1.9:
Time.now.strftime '%$' => '%$'

this make green a mspec test on core/time/strftime_spec.rb

I have some question:
1) i added a check for ruby compatibility >= ruby 1.9, is needed?
2) i removed 'fails:' from
ironruby-tags-19/core/time/strftime_tags.txt is correct?
3) usually code review is in mailing list or github pull request? (so
i dont need to write two times the same questions)
959b1c9d700abfc065f5f40bf5a966a2?d=identicon&s=25 James Schementi (jschementi)
on 2011-08-02 19:52
(Received via mailing list)
On Tue, Aug 2, 2011 at 12:44 PM, Enrico Sada <enrico.sada@gmail.com>
wrote:

>
Looks good. Did the test fail before ths change?


> I have some question:
> 1) i added a check for ruby compatibility >= ruby 1.9, is needed?
>

Not really as we have explicitly decided that IronRuby 1.x targets MRI
1.9,
and doesn't support 1.8.x anymore. However, we didn't remove any of
those
checks for 1.8. It doesn't hurt, but isn't required.


> 2) i removed 'fails:' from
> ironruby-tags-19/core/time/strftime_tags.txt is correct?
>

Yup, you fixed that test so no need for the guard anymore.


> 3) usually code review is in mailing list or github pull request? (so
> i dont need to write two times the same questions)
>

It doesn't matter where you actually write the text, but the pull
request is
needed as well as notifying the mailing list. I say put everything in
the
pull request, and then send the link to the pull request to the mailing
list.
Cb51033949ffccd982ae32c9f890f25a?d=identicon&s=25 Tomas Matousek (Guest)
on 2011-08-02 20:17
(Received via mailing list)
You can remove compatibility check (including the one that's there).
IronRuby only supports MRI 1.9.

Tomas
B3068fb9bae93b2889ffc73feb1b3195?d=identicon&s=25 Enrico Sada (Guest)
on 2011-08-02 20:59
(Received via mailing list)
yes the test was red before.

i will remove the checks for 1.9 (are useless) and update the pull
request.
B3068fb9bae93b2889ffc73feb1b3195?d=identicon&s=25 Enrico Sada (Guest)
on 2011-08-05 18:07
(Received via mailing list)
updated removing checks, now is ready to pull
This topic is locked and can not be replied to.