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 +++++++++++++++++++++++++++++++++++ Initial pass at the new .NET interop test structure. I''d like you guys to look at the organization vs the structure of the tests, and the use of csc in Ruby. * For structure, I am using /concept/action/modifier * For csc, I am going to make it pass the binding and use the binding to encode comments in the C# source file. I can split it into multiple assemblies based on this at a later date. * I am going to do the work to get this running in SNAP as well. -------------- next part -------------- A non-text attachment was scrubbed... Name: interop3.diff Type: application/octet-stream Size: 68361 bytes Desc: interop3.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090306/4d53d1c1/attachment-0001.obj>
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 -------------- next part -------------- A non-text attachment was scrubbed... Name: interop3.diff Type: application/octet-stream Size: 75354 bytes Desc: interop3.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090306/43dde11a/attachment-0001.obj>
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
FYI review: Modified csc.rb to generate arbitrary assemblies and renamed the main generated code to fixtures.generated.cs. JD tfpt review "/shelveset:interop4;REDMOND\jdeville" Comment : * Rearrange .NET tests according to the new test structure * Add the csc and assembly methods to inline C# fixtures into the Ruby code * 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> -----Original Message----- > From: Shri Borde > Sent: Monday, March 09, 2009 5:08 PM > To: Jim Deville; IronRuby External Code Reviewers > Cc: ironruby-core at rubyforge.org > Subject: RE: Code Review: interop3 > > 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 > >-------------- next part -------------- A non-text attachment was scrubbed... Name: interop4.diff Type: application/octet-stream Size: 73778 bytes Desc: interop4.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090311/a0fc67fb/attachment-0001.obj>
Yup, you should update both copies of dev.bat since we do want mspec easily (and similarly) usable with TFS as well. You could combine the two copies if you want. You will have to conditionalize other things like setting of RUBY18_EXE etc as well. You can diff the two copies to see everything that would need to be conditionalized. When I cloned it, it was simpler to clone it since it''s a small file anyway, and my thinking was that I can go back and refactor it if needed once things settles down. -----Original Message----- From: Jim Deville Sent: Wednesday, March 11, 2009 1:50 PM To: Shri Borde; IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: interop3 FYI review: Modified csc.rb to generate arbitrary assemblies and renamed the main generated code to fixtures.generated.cs. JD tfpt review "/shelveset:interop4;REDMOND\jdeville" Comment : * Rearrange .NET tests according to the new test structure * Add the csc and assembly methods to inline C# fixtures into the Ruby code * 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> -----Original Message----- > From: Shri Borde > Sent: Monday, March 09, 2009 5:08 PM > To: Jim Deville; IronRuby External Code Reviewers > Cc: ironruby-core at rubyforge.org > Subject: RE: Code Review: interop3 > > 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 > >