[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string
enrico.sada at gmail.com
Tue Aug 2 14:27:44 EDT 2011
yes the test was red before.
i will remove the checks for 1.9 (are useless) and update the pull request.
On Tue, Aug 2, 2011 at 7:20 PM, Jimmy Schementi <jschementi at gmail.com> wrote:
> On Tue, Aug 2, 2011 at 12:44 PM, Enrico Sada <enrico.sada at gmail.com> wrote:
>> 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
> 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
>> Ironruby-core mailing list
>> Ironruby-core at rubyforge.org
> Ironruby-core mailing list
> Ironruby-core at rubyforge.org
More information about the Ironruby-core