Enrico Sada
2011-Aug-02 16:44 UTC
[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string
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)
Jimmy Schementi
2011-Aug-02 17:20 UTC
[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string
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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20110802/ddf58a65/attachment.html>
Tomas Matousek
2011-Aug-02 17:43 UTC
[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string
You can remove compatibility check (including the one that''s there). IronRuby only supports MRI 1.9. Tomas -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Enrico Sada Sent: Tuesday, August 02, 2011 9:45 AM To: ironruby-core at rubyforge.org Subject: [Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string 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) _______________________________________________ Ironruby-core mailing list Ironruby-core at rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core
Enrico Sada
2011-Aug-02 18:27 UTC
[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string
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 > >
Enrico Sada
2011-Aug-05 15:42 UTC
[Ironruby-core] Code Review - Time#strftime ignore invalid directives on format string
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 >> >> >