# Code Review: mspec-12 **URL:** \ **Category:** IronRuby\ **Created:** [June 27, 2008, 12:54am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556 "2008-06-27T00:54:11Z")\ **Posts on this page:** 6\ **Page:** 1
**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\ **Post date:** [June 27, 2008, 12:54am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556/1 "2008-06-27T00:54:11Z")
tfpt review “/shelveset:mspec-12;REDMOND\jflam” Fixes to enable RubySpecs running in SNAP - Implementation of Kernel#exec - Fixes bugs in Kernel#exit! - calling incorrect overload - Implementation of Kernel#printf - Fixes bug in setting visibility of methods defined through attr\_\* methods (now public) - Adds overloads for File#open to handle name, modenum case and fixes a big in how IO#open was delegating to File constructors - Defines a FileTestOps class (does nothing right now) - Adds a Process#euid method that always returns 0 (need to understand whether detecting admin under windows is possible / realistic using this method) - Adds a RUBY\_PATCHLEVEL constant and sets it to 0 - Fixes bug in exit n where we were failing to return n as exit code - Added a new RubyTestKey.snk which just contains our public key - in preparation for removing transform for SIGNED in to\_svn rake task - A few baseline exclusion changes for existing specs ---
**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\ **Post date:** [June 27, 2008, 1:30am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556/2 "2008-06-27T01:30:26Z")
Test issues: - Instead of testing for ENV[“HOME”] with unless ENV[“HOME”].nil? why not use if ENV[“HOME”] it’s a little more idiomatic. - All of the \_spec.rb.txt files have been renamed to \_tags.txt. You’ll need to fix your changeset for that. - The new rakefile doesn’t allow testing methods directly. Any reason? Also, I’d prefer to keep the test tasks namespaced for organization sake. Can you at least move them into the mspec namespace with the rest of the mspec tasks? I’ll remove redundant tasks later. JD ---
**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\ **Post date:** [June 27, 2008, 1:48am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556/3 "2008-06-27T01:48:00Z")
1. There is some garbage in IronRuby.Tests.csproj. 2. Kernel#printf: It shouldn’t look-up $stdin variable by name. Instead, RbExecContext.StandardOutput should be used. Also “write” should be invoked dynamically. $a = [] class \<\< $stdout def write \*args $a \<\< args end end alias $stdout $foo printf("%d %d", 1, 2) class \<\< STDOUT remove\_method :write end p $a 1. class Foo attr\_accessor :initialize end p Foo.private\_instance\_methods(false) p Foo.public\_instance\_methods(false) [“initialize”] [“initialize=”] Attribute setter is not private. 1. Yaml initializers shouldn’t be empty. Tomas ---
**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\ **Post date:** [June 27, 2008, 5:13am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556/4 "2008-06-27T05:13:49Z")
Thanks for the catches. I’m leaving initialize= as public. I think that it’s likely a bug that attr\_accessor :initialize actually runs (Charlie actually agrees with this). I’ll leave this as is for the time being and open a bug for us to resolve down the road. Yaml.Initializers is empty – strange. I reverted the change, but there’s currently a bug in how ClassInitGenerator generates the code for Yaml.Initializers. I believe Oleg manually edited to fix the change? I’ve also updated the Rakefile to generate the initializers for YAML. I have no idea where the crap that’s in IronRuby.Tests.csproj came from - I’ve reverted that change too. Thanks, -John ---
**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\ **Post date:** [June 27, 2008, 5:16am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556/5 "2008-06-27T05:16:09Z")
Jim D.: > Test issues: > > - Instead of testing for ENV[“HOME”] with > unless ENV[“HOME”].nil? > why not use > if ENV[“HOME”] > it’s a little more idiomatic. Done. > - All of the \_spec.rb.txt files have been renamed to \_tags.txt. You’ll > need to fix your changeset for that. Done. > - The new rakefile doesn’t allow testing methods directly. Any reason? > Also, I’d prefer to keep the test tasks namespaced for organization > sake. Can you at least move them into the mspec namespace with the rest > of the mspec tasks? I’ll remove redundant tasks later. It’s done to keep typing to a minimum. I don’t like having to type more than I have to (and rake regression is already quite long). I replaced rake test to keep things short as well. I realize that folks can define aliases for these, but I’d rather the out of box experience be better than that. Thanks, -John ---
**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\ **Post date:** [June 27, 2008, 8:34am UTC](https://www.ruby-forum.com/t/code-review-mspec-12/141556/6 "2008-06-27T08:34:04Z")
Understood. We do need to watch for a littered namespace though if we are going to keep stuff top level. That’s part of why Rails moved to namespaces. What’s your thought on splitting the Rakefile into a compile.rake and a test.rake, for easier maintenance? JD * * * From: John L. (IRONRUBY) Sent: Thursday, June 26, 2008 8:15 PM To: Jim D.; removed\_email\_address@domain.invalid; IronRuby External Code Reviewers Subject: RE: Code Review: mspec-12 Jim D.: > Test issues: > > - Instead of testing for ENV[“HOME”] with > unless ENV[“HOME”].nil? > why not use > if ENV[“HOME”] > it’s a little more idiomatic. Done. > - All of the \_spec.rb.txt files have been renamed to \_tags.txt. You’ll > need to fix your changeset for that. Done. > - The new rakefile doesn’t allow testing methods directly. Any reason? > Also, I’d prefer to keep the test tasks namespaced for organization > sake. Can you at least move them into the mspec namespace with the rest > of the mspec tasks? I’ll remove redundant tasks later. It’s done to keep typing to a minimum. I don’t like having to type more than I have to (and rake regression is already quite long). I replaced rake test to keep things short as well. I realize that folks can define aliases for these, but I’d rather the out of box experience be better than that. Thanks, -John