Code Review: Update RubySpec


#1

I’m updating RubySpec to the newest revision from Git. Most of this is
boring stuff; just updating files and rebuilding tags. However, a couple
of these commits are code changes to help the specs run smoother.

JD

  •     add ci_files to default.mspec so mspec ci 
    

workshttp://github.com/jredville/ironruby/commit/2ef5e803b6089a3b7b6513de8eb44555f8a8537a

  •     add a filtered function to mspec to simplify 
    

default.mspechttp://github.com/jredville/ironruby/commit/eb9d2047ecee954205fb1d8cd80df7ffaeb4bac2


  •     categorizing uncategorized 
    

specshttp://github.com/jredville/ironruby/commit/0845a3e803688372d9f1959633ad7840f82f7657


  •     removing extra spec 
    

fileshttp://github.com/jredville/ironruby/commit/d973948111456d387e91243e4d8399483fb29fc6

  •     update rubyspec and 
    

mspechttp://github.com/jredville/ironruby/commit/75122c3ade440e28d933c658a95b43acd26e5f9e

  •     add a file with the git sha1 hashes of current mspec and 
    

rubyspechttp://github.com/jredville/ironruby/commit/588dcbfa225156c19c61d882f82e515c5b8f0e73

  •     implement File.umask in order to run the 
    

specshttp://github.com/jredville/ironruby/commit/61ce27f27a510c512095aa3b5ab8329229dce7ef


  •     Implement Process.waitall (needed by Process 
    

specs)http://github.com/jredville/ironruby/commit/f1f148f773111c561e1eea515681799a7559437e


  •     rebuilt ironruby 
    

tagshttp://github.com/jredville/ironruby/commit/d176ec1d4e6d546420e6645554b75c3f94d8e475

The commits marked with *** are more than just infrastructure work.
These are actual code changes that need review.

Thanks

JD


#2

What is umask supposed to do?

Tomas

From: Jim D.
Sent: Tuesday, April 21, 2009 8:12 PM
To: removed_email_address@domain.invalid; IronRuby External Code R.
Subject: Code Review: Update RubySpec

I’m updating RubySpec to the newest revision from Git. Most of this is
boring stuff; just updating files and rebuilding tags. However, a couple
of these commits are code changes to help the specs run smoother.

JD

  •     add ci_files to default.mspec so mspec ci 
    

workshttp://github.com/jredville/ironruby/commit/2ef5e803b6089a3b7b6513de8eb44555f8a8537a

  •     add a filtered function to mspec to simplify 
    

default.mspechttp://github.com/jredville/ironruby/commit/eb9d2047ecee954205fb1d8cd80df7ffaeb4bac2


  •     categorizing uncategorized 
    

specshttp://github.com/jredville/ironruby/commit/0845a3e803688372d9f1959633ad7840f82f7657


  •     removing extra spec 
    

fileshttp://github.com/jredville/ironruby/commit/d973948111456d387e91243e4d8399483fb29fc6

  •     update rubyspec and 
    

mspechttp://github.com/jredville/ironruby/commit/75122c3ade440e28d933c658a95b43acd26e5f9e

  •     add a file with the git sha1 hashes of current mspec and 
    

rubyspechttp://github.com/jredville/ironruby/commit/588dcbfa225156c19c61d882f82e515c5b8f0e73

  •     implement File.umask in order to run the 
    

specshttp://github.com/jredville/ironruby/commit/61ce27f27a510c512095aa3b5ab8329229dce7ef


  •     Implement Process.waitall (needed by Process 
    

specs)http://github.com/jredville/ironruby/commit/f1f148f773111c561e1eea515681799a7559437e


  •     rebuilt ironruby 
    

tagshttp://github.com/jredville/ironruby/commit/d176ec1d4e6d546420e6645554b75c3f94d8e475

The commits marked with *** are more than just infrastructure work.
These are actual code changes that need review.

Thanks

JD


#3

Umask is a mask on file creation. By itself it does nothing more than
what I’ve implemented (that I can tell), but File.open (and other file
creation methods) should look at it to derive the default permissions.
The attached script shows the basic idea, and here’s it’s output:

[9] > ruby .\temp.rb
Umask is 0
100644
Umask is 200
100444
Umask is 400
100644
Umask is 600
100444

If you run it, it will create 4 files (for the 4 tested modes) so you
can see the permissions applied. I looked into adding this support to
File.open, but I’m not certain where to begin (I found public
RuleGenerator Open(), which I don’t know enough about yet).

JD

From: Tomas M.
Sent: Wednesday, April 22, 2009 12:00 AM
To: Jim D.; removed_email_address@domain.invalid; IronRuby External Code
Reviewers
Subject: RE: Code Review: Update RubySpec

What is umask supposed to do?

Tomas

From: Jim D.
Sent: Tuesday, April 21, 2009 8:12 PM
To: removed_email_address@domain.invalid; IronRuby External Code R.
Subject: Code Review: Update RubySpec

I’m updating RubySpec to the newest revision from Git. Most of this is
boring stuff; just updating files and rebuilding tags. However, a couple
of these commits are code changes to help the specs run smoother.

JD

  •     add ci_files to default.mspec so mspec ci 
    

workshttp://github.com/jredville/ironruby/commit/2ef5e803b6089a3b7b6513de8eb44555f8a8537a

  •     add a filtered function to mspec to simplify 
    

default.mspechttp://github.com/jredville/ironruby/commit/eb9d2047ecee954205fb1d8cd80df7ffaeb4bac2


  •     categorizing uncategorized 
    

specshttp://github.com/jredville/ironruby/commit/0845a3e803688372d9f1959633ad7840f82f7657


  •     removing extra spec 
    

fileshttp://github.com/jredville/ironruby/commit/d973948111456d387e91243e4d8399483fb29fc6

  •     update rubyspec and 
    

mspechttp://github.com/jredville/ironruby/commit/75122c3ade440e28d933c658a95b43acd26e5f9e

  •     add a file with the git sha1 hashes of current mspec and 
    

rubyspechttp://github.com/jredville/ironruby/commit/588dcbfa225156c19c61d882f82e515c5b8f0e73

  •     implement File.umask in order to run the 
    

specshttp://github.com/jredville/ironruby/commit/61ce27f27a510c512095aa3b5ab8329229dce7ef


  •     Implement Process.waitall (needed by Process 
    

specs)http://github.com/jredville/ironruby/commit/f1f148f773111c561e1eea515681799a7559437e


  •     rebuilt ironruby 
    

tagshttp://github.com/jredville/ironruby/commit/d176ec1d4e6d546420e6645554b75c3f94d8e475

The commits marked with *** are more than just infrastructure work.
These are actual code changes that need review.

Thanks

JD


#4

Finished offline and F2F with Tomas.

JD

From: Tomas M.
Sent: Wednesday, April 22, 2009 11:33 AM
To: Jim D.; removed_email_address@domain.invalid; IronRuby External Code
Reviewers
Subject: RE: Code Review: Update RubySpec

I see - umask maintains a global state. Any global state maintained by
libraries should be stored on RubyContext, not in static fields. Use
RubyContext.GetOrCreateLibraryData/TryGetLibraryData (if you search the
library you’ll find some use cases). Also defining a struct just to
store an integer seems unnecessary (besides it’s not good to define
structs with mutable state).

Tomas

From: Jim D.
Sent: Wednesday, April 22, 2009 11:09 AM
To: Tomas M.; removed_email_address@domain.invalid; IronRuby External Code
Reviewers
Subject: RE: Code Review: Update RubySpec

Umask is a mask on file creation. By itself it does nothing more than
what I’ve implemented (that I can tell), but File.open (and other file
creation methods) should look at it to derive the default permissions.
The attached script shows the basic idea, and here’s it’s output:

[9] > ruby .\temp.rb
Umask is 0
100644
Umask is 200
100444
Umask is 400
100644
Umask is 600
100444

If you run it, it will create 4 files (for the 4 tested modes) so you
can see the permissions applied. I looked into adding this support to
File.open, but I’m not certain where to begin (I found public
RuleGenerator Open(), which I don’t know enough about yet).

JD

From: Tomas M.
Sent: Wednesday, April 22, 2009 12:00 AM
To: Jim D.; removed_email_address@domain.invalid; IronRuby External Code
Reviewers
Subject: RE: Code Review: Update RubySpec

What is umask supposed to do?

Tomas

From: Jim D.
Sent: Tuesday, April 21, 2009 8:12 PM
To: removed_email_address@domain.invalid; IronRuby External Code R.
Subject: Code Review: Update RubySpec

I’m updating RubySpec to the newest revision from Git. Most of this is
boring stuff; just updating files and rebuilding tags. However, a couple
of these commits are code changes to help the specs run smoother.

JD

  •     add ci_files to default.mspec so mspec ci 
    

workshttp://github.com/jredville/ironruby/commit/2ef5e803b6089a3b7b6513de8eb44555f8a8537a

  •     add a filtered function to mspec to simplify 
    

default.mspechttp://github.com/jredville/ironruby/commit/eb9d2047ecee954205fb1d8cd80df7ffaeb4bac2


  •     categorizing uncategorized 
    

specshttp://github.com/jredville/ironruby/commit/0845a3e803688372d9f1959633ad7840f82f7657


  •     removing extra spec 
    

fileshttp://github.com/jredville/ironruby/commit/d973948111456d387e91243e4d8399483fb29fc6

  •     update rubyspec and 
    

mspechttp://github.com/jredville/ironruby/commit/75122c3ade440e28d933c658a95b43acd26e5f9e

  •     add a file with the git sha1 hashes of current mspec and 
    

rubyspechttp://github.com/jredville/ironruby/commit/588dcbfa225156c19c61d882f82e515c5b8f0e73

  •     implement File.umask in order to run the 
    

specshttp://github.com/jredville/ironruby/commit/61ce27f27a510c512095aa3b5ab8329229dce7ef


  •     Implement Process.waitall (needed by Process 
    

specs)http://github.com/jredville/ironruby/commit/f1f148f773111c561e1eea515681799a7559437e


  •     rebuilt ironruby 
    

tagshttp://github.com/jredville/ironruby/commit/d176ec1d4e6d546420e6645554b75c3f94d8e475

The commits marked with *** are more than just infrastructure work.
These are actual code changes that need review.

Thanks

JD


#5

I see - umask maintains a global state. Any global state maintained by
libraries should be stored on RubyContext, not in static fields. Use
RubyContext.GetOrCreateLibraryData/TryGetLibraryData (if you search the
library you’ll find some use cases). Also defining a struct just to
store an integer seems unnecessary (besides it’s not good to define
structs with mutable state).

Tomas

From: Jim D.
Sent: Wednesday, April 22, 2009 11:09 AM
To: Tomas M.; removed_email_address@domain.invalid; IronRuby External Code
Reviewers
Subject: RE: Code Review: Update RubySpec

Umask is a mask on file creation. By itself it does nothing more than
what I’ve implemented (that I can tell), but File.open (and other file
creation methods) should look at it to derive the default permissions.
The attached script shows the basic idea, and here’s it’s output:

[9] > ruby .\temp.rb
Umask is 0
100644
Umask is 200
100444
Umask is 400
100644
Umask is 600
100444

If you run it, it will create 4 files (for the 4 tested modes) so you
can see the permissions applied. I looked into adding this support to
File.open, but I’m not certain where to begin (I found public
RuleGenerator Open(), which I don’t know enough about yet).

JD

From: Tomas M.
Sent: Wednesday, April 22, 2009 12:00 AM
To: Jim D.; removed_email_address@domain.invalid; IronRuby External Code
Reviewers
Subject: RE: Code Review: Update RubySpec

What is umask supposed to do?

Tomas

From: Jim D.
Sent: Tuesday, April 21, 2009 8:12 PM
To: removed_email_address@domain.invalid; IronRuby External Code R.
Subject: Code Review: Update RubySpec

I’m updating RubySpec to the newest revision from Git. Most of this is
boring stuff; just updating files and rebuilding tags. However, a couple
of these commits are code changes to help the specs run smoother.

JD

  •     add ci_files to default.mspec so mspec ci 
    

workshttp://github.com/jredville/ironruby/commit/2ef5e803b6089a3b7b6513de8eb44555f8a8537a

  •     add a filtered function to mspec to simplify 
    

default.mspechttp://github.com/jredville/ironruby/commit/eb9d2047ecee954205fb1d8cd80df7ffaeb4bac2


  •     categorizing uncategorized 
    

specshttp://github.com/jredville/ironruby/commit/0845a3e803688372d9f1959633ad7840f82f7657


  •     removing extra spec 
    

fileshttp://github.com/jredville/ironruby/commit/d973948111456d387e91243e4d8399483fb29fc6

  •     update rubyspec and 
    

mspechttp://github.com/jredville/ironruby/commit/75122c3ade440e28d933c658a95b43acd26e5f9e

  •     add a file with the git sha1 hashes of current mspec and 
    

rubyspechttp://github.com/jredville/ironruby/commit/588dcbfa225156c19c61d882f82e515c5b8f0e73

  •     implement File.umask in order to run the 
    

specshttp://github.com/jredville/ironruby/commit/61ce27f27a510c512095aa3b5ab8329229dce7ef


  •     Implement Process.waitall (needed by Process 
    

specs)http://github.com/jredville/ironruby/commit/f1f148f773111c561e1eea515681799a7559437e


  •     rebuilt ironruby 
    

tagshttp://github.com/jredville/ironruby/commit/d176ec1d4e6d546420e6645554b75c3f94d8e475

The commits marked with *** are more than just infrastructure work.
These are actual code changes that need review.

Thanks

JD