# Code Review - Time#strftime ignore invalid directives on format string

**URL:** <https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225>\
**Category:** IronRuby\
**Created:** [August 2, 2011, 7:07pm UTC](https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225 "2011-08-02T19:07:06Z")\
**Posts on this page:** 5\
**Page:** 1

<div class="post-metadata">

**Author:** ![luislavena](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/l/0ea827/32.png) [@luislavena](https://www.ruby-forum.com/u/luislavena)\
**Post date:** [August 2, 2011, 7:07pm UTC](https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225/1 "2011-08-02T19:07:06Z")

</div>

i asked a pull request ( [Time#strftime - any text not listed as a directive in the given format st by enricosada · Pull Request #28 · IronLanguages/main · GitHub](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)

---

<div class="post-metadata">

**Author:** ![estro](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/e/838e76/32.png) [@estro](https://www.ruby-forum.com/u/estro)\
**Post date:** [August 2, 2011, 8:17pm UTC](https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225/2 "2011-08-02T20:17:37Z")

</div>

You can remove compatibility check (including the one that’s there).  
IronRuby only supports MRI 1.9.

Tomas

---

<div class="post-metadata">

**Author:** ![estro](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/e/838e76/32.png) [@estro](https://www.ruby-forum.com/u/estro)\
**Post date:** [August 2, 2011, 7:52pm UTC](https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225/3 "2011-08-02T19:52:47Z")

</div>

On Tue, Aug 2, 2011 at 12:44 PM, Enrico S. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid)  
wrote:

> 

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.

> 1. 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.

> 1. 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.

---

<div class="post-metadata">

**Author:** ![estro](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/e/838e76/32.png) [@estro](https://www.ruby-forum.com/u/estro)\
**Post date:** [August 5, 2011, 6:07pm UTC](https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225/4 "2011-08-05T18:07:15Z")

</div>

updated removing checks, now is ready to pull

---

<div class="post-metadata">

**Author:** ![estro](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/e/838e76/32.png) [@estro](https://www.ruby-forum.com/u/estro)\
**Post date:** [August 2, 2011, 8:59pm UTC](https://www.ruby-forum.com/t/code-review-time-strftime-ignore-invalid-directives-on-format-string/210225/5 "2011-08-02T20:59:02Z")

</div>

yes the test was red before.

i will remove the checks for 1.9 (are useless) and update the pull  
request.