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 -------------- next part -------------- A non-text attachment was scrubbed... Name: mspec-12.diff Type: application/octet-stream Size: 50999 bytes Desc: mspec-12.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20080626/c742c7f3/attachment-0001.obj>
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
> -----Original Message-----
> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-
> bounces at rubyforge.org] On Behalf Of John Lam (IRONRUBY)
> Sent: Thursday, June 26, 2008 3:54 PM
> To: IronRuby External Code Reviewers
> Cc: ironruby-core at rubyforge.org
> Subject: [Ironruby-core] Code Review: mspec-12
>
> 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
>
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
3)
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.
4) Yaml initializers shouldn''t be empty.
Tomas
-----Original Message-----
From: John Lam (IRONRUBY)
Sent: Thursday, June 26, 2008 3:54 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: mspec-12
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
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> -----Original Message----- > From: Tomas Matousek > Sent: Thursday, June 26, 2008 4:47 PM > To: John Lam (IRONRUBY); IronRuby External Code Reviewers > Cc: ironruby-core at rubyforge.org > Subject: RE: Code Review: mspec-12 > > 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 > > 3) > 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. > > 4) Yaml initializers shouldn''t be empty. > > Tomas > > > -----Original Message----- > From: John Lam (IRONRUBY) > Sent: Thursday, June 26, 2008 3:54 PM > To: IronRuby External Code Reviewers > Cc: ironruby-core at rubyforge.org > Subject: Code Review: mspec-12 > > 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 >
Jim Deville:> 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
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 Lam (IRONRUBY) Sent: Thursday, June 26, 2008 8:15 PM To: Jim Deville; ironruby-core at rubyforge.org; IronRuby External Code Reviewers Subject: RE: Code Review: mspec-12 Jim Deville:> 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