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

Enrico Sada enrico.sada at gmail.com
Fri Aug 5 11:42:50 EDT 2011


updated removing checks, now is ready to pull

On Tue, Aug 2, 2011 at 8:27 PM, Enrico Sada <enrico.sada at gmail.com> wrote:
> 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