Forum: IronRuby Code Review: Various .NET interop tests

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
Jim D. (Guest)
on 2009-03-27 20:46
(Received via mailing list)
This is one of the weak areas for our Git setup. I have multiple commits
that need review. I honestly feel that Git needs the concept of a push
object, so I could say look at this, or these pushes. So, options to
help you review:

*         Click on each of these links and leave comments

*         Use git however you want

*         Use Git Extensions (if you clone
git://github.com/jredville/ironruby.git, and setup Git Extensions, you
should be able to click on each commit, and each file at your leisure)

*         GitDiff 17de3f82 8a54299 (Unfortuneately, this will include
commits and files from IronRuby's main repo's pushes)



We're trying to figure something better out, including possibly using
Review Board (http://review-board.org). So, now for the actual review:





This a some work on Generic Method specs, as well as more general work
on a first pass of the test plan
(http://ironruby.net/.NET_Testing_plans).





*         modify .gitignore to ignore Bin in addition to
bin<http://github.com/jredville/ironruby/commit/f1ae28...

*         add dlr_config to IronRuby
object<http://github.com/jredville/ironruby/commit/b210fa...

*         csc describe handles shared specs (multiple arguments) and we
now emit
<http://github.com/jredville/ironruby/commit/c863df...

#line pragmas instead of
comments<http://github.com/jredville/ironruby/commit/c863df...

*         adding some generic tests and fixing tags, also make
default.mspec load
<http://github.com/jredville/ironruby/commit/33916d...

ir.exe instead of
ir.cmd<http://github.com/jredville/ironruby/commit/33916d...

*         adding some constrained generic
specs<http://github.com/jredville/ironruby/commit/2e151d...

*         added generic error messages specs. Fixed tag
locations<http://github.com/jredville/ironruby/commit/b0f286...

*         split pragma warning to make sure I do not disable unintended
warnings.
<http://github.com/jredville/ironruby/commit/8167bd...

Refactor conflicting
methods<http://github.com/jredville/ironruby/commit/8167bd...

*         added class param and conflicting type param
specs<http://github.com/jredville/ironruby/commit/481998...

*         adding specs for ruby classes with type constraints and for a
secondary
<http://github.com/jredville/ironruby/commit/9f40c7...

(base class) type
constraint)<http://github.com/jredville/ironruby/commit/9f40c7...

*         array conversion
specs<http://github.com/jredville/ironruby/commit/f47f33...

*         array instantiation
specs<http://github.com/jredville/ironruby/commit/183983...

*         redid IronRuby.dlr_config after Tomas' IronRuby
changes<http://github.com/jredville/ironruby/commit/a446fb...

*         adding a default conversion spec and a little bit of
refactoring<http://github.com/jredville/ironruby/commit/b43afd...

*         more array
tests<http://github.com/jredville/ironruby/commit/600a3a...

*         spec for a static method caching bug i
found<http://github.com/jredville/ironruby/commit/2c7f69...

*         spec method overriding maintains .NET
supermethod<http://github.com/jredville/ironruby/commit/82e5a3...

*         refactor to add some metaclass
helpers<http://github.com/jredville/ironruby/commit/4c9d1d...

*         class instantiation
specs<http://github.com/jredville/ironruby/commit/fa6de4...

*         some more class instantiation
specs<http://github.com/jredville/ironruby/commit/42b088...

*         sealed class instantiation
specs<http://github.com/jredville/ironruby/commit/21eaf4...

*         generic instantiation
specs<http://github.com/jredville/ironruby/commit/5e8cf7...

*         make GenericClass have a method so it isn't
EmptyGenericClass<http://github.com/jredville/ironruby/commit/47b05e...

*         more generic instantiation
specs<http://github.com/jredville/ironruby/commit/8a5429...

*
Jimmy S. (Guest)
on 2009-03-27 23:28
(Received via mailing list)
Looks good to me. I left comments where appropriate, but nothing
stopping you from checking in.

From: Jim D.
Sent: Friday, March 27, 2009 11:40 AM
To: IronRuby External Code R.
Cc: removed_email_address@domain.invalid
Subject: Code Review: Various .NET interop tests


This is one of the weak areas for our Git setup. I have multiple commits
that need review. I honestly feel that Git needs the concept of a push
object, so I could say look at this, or these pushes. So, options to
help you review:

*         Click on each of these links and leave comments

*         Use git however you want

*         Use Git Extensions (if you clone
git://github.com/jredville/ironruby.git, and setup Git Extensions, you
should be able to click on each commit, and each file at your leisure)

*         GitDiff 17de3f82 8a54299 (Unfortuneately, this will include
commits and files from IronRuby's main repo's pushes)



We're trying to figure something better out, including possibly using
Review Board (http://review-board.org). So, now for the actual review:





This a some work on Generic Method specs, as well as more general work
on a first pass of the test plan
(http://ironruby.net/.NET_Testing_plans).





*         modify .gitignore to ignore Bin in addition to
bin<http://github.com/jredville/ironruby/commit/f1ae28...

*         add dlr_config to IronRuby
object<http://github.com/jredville/ironruby/commit/b210fa...

*         csc describe handles shared specs (multiple arguments) and we
now emit
<http://github.com/jredville/ironruby/commit/c863df...

#line pragmas instead of
comments<http://github.com/jredville/ironruby/commit/c863df...

*         adding some generic tests and fixing tags, also make
default.mspec load
<http://github.com/jredville/ironruby/commit/33916d...

ir.exe instead of
ir.cmd<http://github.com/jredville/ironruby/commit/33916d...

*         adding some constrained generic
specs<http://github.com/jredville/ironruby/commit/2e151d...

*         added generic error messages specs. Fixed tag
locations<http://github.com/jredville/ironruby/commit/b0f286...

*         split pragma warning to make sure I do not disable unintended
warnings.
<http://github.com/jredville/ironruby/commit/8167bd...

Refactor conflicting
methods<http://github.com/jredville/ironruby/commit/8167bd...

*         added class param and conflicting type param
specs<http://github.com/jredville/ironruby/commit/481998...

*         adding specs for ruby classes with type constraints and for a
secondary
<http://github.com/jredville/ironruby/commit/9f40c7...

(base class) type
constraint)<http://github.com/jredville/ironruby/commit/9f40c7...

*         array conversion
specs<http://github.com/jredville/ironruby/commit/f47f33...

*         array instantiation
specs<http://github.com/jredville/ironruby/commit/183983...

*         redid IronRuby.dlr_config after Tomas' IronRuby
changes<http://github.com/jredville/ironruby/commit/a446fb...

*         adding a default conversion spec and a little bit of
refactoring<http://github.com/jredville/ironruby/commit/b43afd...

*         more array
tests<http://github.com/jredville/ironruby/commit/600a3a...

*         spec for a static method caching bug i
found<http://github.com/jredville/ironruby/commit/2c7f69...

*         spec method overriding maintains .NET
supermethod<http://github.com/jredville/ironruby/commit/82e5a3...

*         refactor to add some metaclass
helpers<http://github.com/jredville/ironruby/commit/4c9d1d...

*         class instantiation
specs<http://github.com/jredville/ironruby/commit/fa6de4...

*         some more class instantiation
specs<http://github.com/jredville/ironruby/commit/42b088...

*         sealed class instantiation
specs<http://github.com/jredville/ironruby/commit/21eaf4...

*         generic instantiation
specs<http://github.com/jredville/ironruby/commit/5e8cf7...

*         make GenericClass have a method so it isn't
EmptyGenericClass<http://github.com/jredville/ironruby/commit/47b05e...

*         more generic instantiation
specs<http://github.com/jredville/ironruby/commit/8a5429...

*
Jim D. (Guest)
on 2009-03-27 23:56
(Received via mailing list)
Cool. As part of pulling this into TFS, I'm going to add ClrAssembly to
Ruby.sln so that we build it from VS.

JD

From: Jimmy S.
Sent: Friday, March 27, 2009 1:22 PM
To: Jim D.; IronRuby External Code R.
Cc: removed_email_address@domain.invalid
Subject: RE: Code Review: Various .NET interop tests

Looks good to me. I left comments where appropriate, but nothing
stopping you from checking in.

From: Jim D.
Sent: Friday, March 27, 2009 11:40 AM
To: IronRuby External Code R.
Cc: removed_email_address@domain.invalid
Subject: Code Review: Various .NET interop tests


This is one of the weak areas for our Git setup. I have multiple commits
that need review. I honestly feel that Git needs the concept of a push
object, so I could say look at this, or these pushes. So, options to
help you review:

*         Click on each of these links and leave comments

*         Use git however you want

*         Use Git Extensions (if you clone
git://github.com/jredville/ironruby.git, and setup Git Extensions, you
should be able to click on each commit, and each file at your leisure)

*         GitDiff 17de3f82 8a54299 (Unfortuneately, this will include
commits and files from IronRuby's main repo's pushes)



We're trying to figure something better out, including possibly using
Review Board (http://review-board.org). So, now for the actual review:





This a some work on Generic Method specs, as well as more general work
on a first pass of the test plan
(http://ironruby.net/.NET_Testing_plans).





*         modify .gitignore to ignore Bin in addition to
bin<http://github.com/jredville/ironruby/commit/f1ae28...

*         add dlr_config to IronRuby
object<http://github.com/jredville/ironruby/commit/b210fa...

*         csc describe handles shared specs (multiple arguments) and we
now emit
<http://github.com/jredville/ironruby/commit/c863df...

#line pragmas instead of
comments<http://github.com/jredville/ironruby/commit/c863df...

*         adding some generic tests and fixing tags, also make
default.mspec load
<http://github.com/jredville/ironruby/commit/33916d...

ir.exe instead of
ir.cmd<http://github.com/jredville/ironruby/commit/33916d...

*         adding some constrained generic
specs<http://github.com/jredville/ironruby/commit/2e151d...

*         added generic error messages specs. Fixed tag
locations<http://github.com/jredville/ironruby/commit/b0f286...

*         split pragma warning to make sure I do not disable unintended
warnings.
<http://github.com/jredville/ironruby/commit/8167bd...

Refactor conflicting
methods<http://github.com/jredville/ironruby/commit/8167bd...

*         added class param and conflicting type param
specs<http://github.com/jredville/ironruby/commit/481998...

*         adding specs for ruby classes with type constraints and for a
secondary
<http://github.com/jredville/ironruby/commit/9f40c7...

(base class) type
constraint)<http://github.com/jredville/ironruby/commit/9f40c7...

*         array conversion
specs<http://github.com/jredville/ironruby/commit/f47f33...

*         array instantiation
specs<http://github.com/jredville/ironruby/commit/183983...

*         redid IronRuby.dlr_config after Tomas' IronRuby
changes<http://github.com/jredville/ironruby/commit/a446fb...

*         adding a default conversion spec and a little bit of
refactoring<http://github.com/jredville/ironruby/commit/b43afd...

*         more array
tests<http://github.com/jredville/ironruby/commit/600a3a...

*         spec for a static method caching bug i
found<http://github.com/jredville/ironruby/commit/2c7f69...

*         spec method overriding maintains .NET
supermethod<http://github.com/jredville/ironruby/commit/82e5a3...

*         refactor to add some metaclass
helpers<http://github.com/jredville/ironruby/commit/4c9d1d...

*         class instantiation
specs<http://github.com/jredville/ironruby/commit/fa6de4...

*         some more class instantiation
specs<http://github.com/jredville/ironruby/commit/42b088...

*         sealed class instantiation
specs<http://github.com/jredville/ironruby/commit/21eaf4...

*         generic instantiation
specs<http://github.com/jredville/ironruby/commit/5e8cf7...

*         make GenericClass have a method so it isn't
EmptyGenericClass<http://github.com/jredville/ironruby/commit/47b05e...

*         more generic instantiation
specs<http://github.com/jredville/ironruby/commit/8a5429...

*
Shri B. (Guest)
on 2009-03-31 03:44
(Received via mailing list)
Could you have avoided doing pulls from the main repo? Not ideal but
given the tools we have, we will have to figure out a solution that
balances all interests.

You could have a separate branch for active work if you do need pulls
for main for some other reason like doing code reviews for others, just
keeping on top of whats going on, etc.

Also, if you can manage to avoid pulls from the main repo, should
rebasing be recommended? You want to do frequent pushes maily to back up
your changes in the cloud, and so rebasing sounds like a good option
once you are finally happy with the changes and want to push it into
your master branch. It will be easier to browse file histories...

Also, I suppose this issue is not unique to IronRuby. How do other
projects deal with code reviews, keeping file history less noisy, etc?

Thanks,
Shri

From: Jim D.
Sent: Friday, March 27, 2009 11:40 AM
To: IronRuby External Code R.
Cc: removed_email_address@domain.invalid
Subject: Code Review: Various .NET interop tests


This is one of the weak areas for our Git setup. I have multiple commits
that need review. I honestly feel that Git needs the concept of a push
object, so I could say look at this, or these pushes. So, options to
help you review:

*         Click on each of these links and leave comments

*         Use git however you want

*         Use Git Extensions (if you clone
git://github.com/jredville/ironruby.git, and setup Git Extensions, you
should be able to click on each commit, and each file at your leisure)

*         GitDiff 17de3f82 8a54299 (Unfortuneately, this will include
commits and files from IronRuby's main repo's pushes)



We're trying to figure something better out, including possibly using
Review Board (http://review-board.org). So, now for the actual review:





This a some work on Generic Method specs, as well as more general work
on a first pass of the test plan
(http://ironruby.net/.NET_Testing_plans).





*         modify .gitignore to ignore Bin in addition to
bin<http://github.com/jredville/ironruby/commit/f1ae28...

*         add dlr_config to IronRuby
object<http://github.com/jredville/ironruby/commit/b210fa...

*         csc describe handles shared specs (multiple arguments) and we
now emit
<http://github.com/jredville/ironruby/commit/c863df...

#line pragmas instead of
comments<http://github.com/jredville/ironruby/commit/c863df...

*         adding some generic tests and fixing tags, also make
default.mspec load
<http://github.com/jredville/ironruby/commit/33916d...

ir.exe instead of
ir.cmd<http://github.com/jredville/ironruby/commit/33916d...

*         adding some constrained generic
specs<http://github.com/jredville/ironruby/commit/2e151d...

*         added generic error messages specs. Fixed tag
locations<http://github.com/jredville/ironruby/commit/b0f286...

*         split pragma warning to make sure I do not disable unintended
warnings.
<http://github.com/jredville/ironruby/commit/8167bd...

Refactor conflicting
methods<http://github.com/jredville/ironruby/commit/8167bd...

*         added class param and conflicting type param
specs<http://github.com/jredville/ironruby/commit/481998...

*         adding specs for ruby classes with type constraints and for a
secondary
<http://github.com/jredville/ironruby/commit/9f40c7...

(base class) type
constraint)<http://github.com/jredville/ironruby/commit/9f40c7...

*         array conversion
specs<http://github.com/jredville/ironruby/commit/f47f33...

*         array instantiation
specs<http://github.com/jredville/ironruby/commit/183983...

*         redid IronRuby.dlr_config after Tomas' IronRuby
changes<http://github.com/jredville/ironruby/commit/a446fb...

*         adding a default conversion spec and a little bit of
refactoring<http://github.com/jredville/ironruby/commit/b43afd...

*         more array
tests<http://github.com/jredville/ironruby/commit/600a3a...

*         spec for a static method caching bug i
found<http://github.com/jredville/ironruby/commit/2c7f69...

*         spec method overriding maintains .NET
supermethod<http://github.com/jredville/ironruby/commit/82e5a3...

*         refactor to add some metaclass
helpers<http://github.com/jredville/ironruby/commit/4c9d1d...

*         class instantiation
specs<http://github.com/jredville/ironruby/commit/fa6de4...

*         some more class instantiation
specs<http://github.com/jredville/ironruby/commit/42b088...

*         sealed class instantiation
specs<http://github.com/jredville/ironruby/commit/21eaf4...

*         generic instantiation
specs<http://github.com/jredville/ironruby/commit/5e8cf7...

*         make GenericClass have a method so it isn't
EmptyGenericClass<http://github.com/jredville/ironruby/commit/47b05e...

*         more generic instantiation
specs<http://github.com/jredville/ironruby/commit/8a5429...

*
Jimmy S. (Guest)
on 2009-03-31 06:06
(Received via mailing list)
As a slight variation/summarization, I'd suggest making a branch for any
specific change you'll be making, especially if it'll be made up of
multiple commits. Pulls could be made on master, and merged into the
branch as needed. Then, you could rebase and present the change as one
commit to your master branch, which then could go out for code review.

Though, I liked having many small changes, since the modified-file-count
was small per link, so it was almost like having a file list. Though
github makes it a bit irritating that it doesn't show the diffs of new
or deleted files, so you have to click on each one of those.

~js

From: removed_email_address@domain.invalid
[mailto:removed_email_address@domain.invalid] On Behalf Of Shri B.
Sent: Monday, March 30, 2009 4:44 PM
To: Jim D.
Cc: removed_email_address@domain.invalid
Subject: [Ironruby-core] Mapping multiple pushes to a single review

Could you have avoided doing pulls from the main repo? Not ideal but
given the tools we have, we will have to figure out a solution that
balances all interests.

You could have a separate branch for active work if you do need pulls
for main for some other reason like doing code reviews for others, just
keeping on top of whats going on, etc.

Also, if you can manage to avoid pulls from the main repo, should
rebasing be recommended? You want to do frequent pushes maily to back up
your changes in the cloud, and so rebasing sounds like a good option
once you are finally happy with the changes and want to push it into
your master branch. It will be easier to browse file histories...

Also, I suppose this issue is not unique to IronRuby. How do other
projects deal with code reviews, keeping file history less noisy, etc?

Thanks,
Shri

From: Jim D.
Sent: Friday, March 27, 2009 11:40 AM
To: IronRuby External Code R.
Cc: removed_email_address@domain.invalid
Subject: Code Review: Various .NET interop tests


This is one of the weak areas for our Git setup. I have multiple commits
that need review. I honestly feel that Git needs the concept of a push
object, so I could say look at this, or these pushes. So, options to
help you review:

*         Click on each of these links and leave comments

*         Use git however you want

*         Use Git Extensions (if you clone
git://github.com/jredville/ironruby.git, and setup Git Extensions, you
should be able to click on each commit, and each file at your leisure)

*         GitDiff 17de3f82 8a54299 (Unfortuneately, this will include
commits and files from IronRuby's main repo's pushes)



We're trying to figure something better out, including possibly using
Review Board (http://review-board.org). So, now for the actual review:





This a some work on Generic Method specs, as well as more general work
on a first pass of the test plan
(http://ironruby.net/.NET_Testing_plans).





*         modify .gitignore to ignore Bin in addition to
bin<http://github.com/jredville/ironruby/commit/f1ae28...

*         add dlr_config to IronRuby
object<http://github.com/jredville/ironruby/commit/b210fa...

*         csc describe handles shared specs (multiple arguments) and we
now emit
<http://github.com/jredville/ironruby/commit/c863df...

#line pragmas instead of
comments<http://github.com/jredville/ironruby/commit/c863df...

*         adding some generic tests and fixing tags, also make
default.mspec load
<http://github.com/jredville/ironruby/commit/33916d...

ir.exe instead of
ir.cmd<http://github.com/jredville/ironruby/commit/33916d...

*         adding some constrained generic
specs<http://github.com/jredville/ironruby/commit/2e151d...

*         added generic error messages specs. Fixed tag
locations<http://github.com/jredville/ironruby/commit/b0f286...

*         split pragma warning to make sure I do not disable unintended
warnings.
<http://github.com/jredville/ironruby/commit/8167bd...

Refactor conflicting
methods<http://github.com/jredville/ironruby/commit/8167bd...

*         added class param and conflicting type param
specs<http://github.com/jredville/ironruby/commit/481998...

*         adding specs for ruby classes with type constraints and for a
secondary
<http://github.com/jredville/ironruby/commit/9f40c7...

(base class) type
constraint)<http://github.com/jredville/ironruby/commit/9f40c7...

*         array conversion
specs<http://github.com/jredville/ironruby/commit/f47f33...

*         array instantiation
specs<http://github.com/jredville/ironruby/commit/183983...

*         redid IronRuby.dlr_config after Tomas' IronRuby
changes<http://github.com/jredville/ironruby/commit/a446fb...

*         adding a default conversion spec and a little bit of
refactoring<http://github.com/jredville/ironruby/commit/b43afd...

*         more array
tests<http://github.com/jredville/ironruby/commit/600a3a...

*         spec for a static method caching bug i
found<http://github.com/jredville/ironruby/commit/2c7f69...

*         spec method overriding maintains .NET
supermethod<http://github.com/jredville/ironruby/commit/82e5a3...

*         refactor to add some metaclass
helpers<http://github.com/jredville/ironruby/commit/4c9d1d...

*         class instantiation
specs<http://github.com/jredville/ironruby/commit/fa6de4...

*         some more class instantiation
specs<http://github.com/jredville/ironruby/commit/42b088...

*         sealed class instantiation
specs<http://github.com/jredville/ironruby/commit/21eaf4...

*         generic instantiation
specs<http://github.com/jredville/ironruby/commit/5e8cf7...

*         make GenericClass have a method so it isn't
EmptyGenericClass<http://github.com/jredville/ironruby/commit/47b05e...

*         more generic instantiation
specs<http://github.com/jredville/ironruby/commit/8a5429...

*
This topic is locked and can not be replied to.