[Ironruby-core] Code Review: interop3

Shri Borde Shri.Borde at microsoft.com
Mon Mar 9 20:07:39 EDT 2009


Looks good. About organization, I gave F2F feedback. Small comments about the nitty gritty...

In assembly/access/dependencies1, can A.dll and B.dll also be generated using some overloaded version of csc which takes an assembly name? Not a big deal for now since there are only two assemblies checked in, but if you are going to need more, you might as well use the csc infrastructure which you are building up anyway.

There is Tests\interop\assembly.cs as well as Tests\interop\assembly folder. Would be nice if the former was called Fixtures.cs or something like that to disambiguate the two.

Could you add a comment to the file saying it is generated by a script, or name it as foo.Generated.cs so it obvious that you should not edit it by hand?

Thanks,
Shri


-----Original Message-----
From: Jim Deville 
Sent: Friday, March 06, 2009 11:13 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: interop3

  tfpt review "/shelveset:interop3;REDMOND\jdeville"
  Comment  : 
  * Rearrange .NET tests according to the new test structure
  * Add the csc method to inline C# fixtures into the Ruby code
  ** Should csc.bat be in a path'd scripts directory? Or is it okay having to specifiy a path to it?
  * Modify both dev.bats to add Merlin\External\Languages\IronRuby\mspec\mspec\bin to %PATH% 
  * Modify IronRuby's dev.bat to setup ~/.mspecrc if ~/.mspecrc doesn't exist.
  ** Should this modification be in both? If so, why not have one Dev.bat that conditionally loads internal alias's?
  * Make the default.mspec, which becomes ~/.mspecrc, simply load our default configuration file from Merlin\External\Languages\IronRuby\mspec\default.mspec
  




More information about the Ironruby-core mailing list