[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string

Enrico Sada 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
> list.
>
>>
>> _______________________________________________
>> Ironruby-core mailing list
>> Ironruby-core at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/ironruby-core
>
>
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core at rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
>
>


More information about the Ironruby-core mailing list