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 <jscheme...@gmail.com> wrote: > On Tue, Aug 2, 2011 at 12:44 PM, Enrico Sada <enrico.s...@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@rubyforge.org >> http://rubyforge.org/mailman/listinfo/ironruby-core > > > _______________________________________________ > Ironruby-core mailing list > Ironruby-core@rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core > > _______________________________________________ Ironruby-core mailing list Ironruby-core@rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core
