[Ironruby-core] Code Review: fixes-2

John Lam (IRONRUBY) jflam at microsoft.com
Wed Jul 9 01:30:53 EDT 2008


Comments inline:

Thanks,
-John


> -----Original Message-----
> From: Jim Deville
> Sent: Tuesday, July 08, 2008 10:21 PM
> To: ironruby-core at rubyforge.org; John Lam (IRONRUBY); Oleg Tkachenko;
> Tomas Matousek
> Cc: Srivatsn Narayanan
> Subject: RE: Code Review: fixes-2
>
> I've included Srivatsn since he will be covering for me when I'm on
> paternity leave.
>
> Context.rb:
> None of the changes in context are using the -B option, so they aren't
> using the default.mspec, unless, perhaps, you have it in the default
> search locations for mspec.

Thanks for the catch.

> I've been looking more, and running via mspec, instead of the direct
> mspec-{ci,tag,run} commands also can affect which specs run. Some specs
> are guarded for if the runner is MSpec or RSpec, using that env.
> Variable (see mspec/lib/mspec/guards/runner.rb). I'd also expect more
> config to move into mspec, so we probably want to keep using it.

Not sure what this means? I had trouble getting the target stuff working in the past, which is why I'm using the direct commands.

> I also wonder why you removed the common command that removed
> repetition when preparing the command line.

This wound up making it much more difficult to tell at a glance what was going on - there was magic inside of that method. I prefer to see the entire command at a glance which is what the original solution did.

> Default.mspec.rb:
> I purposely left core and lang active in default.mspec to allow one
> command to run all passing tests. Do we want that changed?    Target
> should also stay in to allow mspec to be run without requiring the -t
> target.

I don't think that enabling core at this point makes sense since none of the test running infrastructure lets you selectively 'run' a particular core test like we do with the libs. I think we should think some more about this before we enable the core specs.

> Rakefile:
> I don't see the point in changing path_to_ir to iruby, isn't path_to_ir
> already a string?

iruby is a local variable, path_to_ir is a method call. iruby will be used in the future to pass a -X:Interpret flag to run the specs under Tomas' beauty new interpreter.

> Runfirst.cmd:
> I thought it made sense to keep the config file both in ~, any reason
> why not?

Because the relative paths inside of default.mspec wouldn't line up otherwise. Besides, the default locations of the rubyspec, mspec, and ironruby-tags projects all assume a dev subdirectory under home.




More information about the Ironruby-core mailing list