tfpt review "/shelveset:bigdecimal-2;REDMOND\jflam" Comment : BigDecimal contribution from Peter Bacon Darwin -------------- next part -------------- A non-text attachment was scrubbed... Name: bigdecimal-2.diff Type: application/octet-stream Size: 243833 bytes Desc: bigdecimal-2.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20081003/80af0264/attachment-0001.obj>
Code looks good. There might have been less churn to have CodeContext on each method now so that we can use it to get a Config later -- but there''s also something to be said for making it work first. Style consistency issues: The line endings in bigdecimal.cs appear to be a mix of LF and CRLF. They should all be CRLF. We tend to prefer not to say "this.varname = x"; if the symbol name conflicts, it should be changed if possible. -----Original Message----- From: John Lam (IRONRUBY) Sent: Friday, October 03, 2008 3:24 PM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: Code Review: bigdecimal-2 tfpt review "/shelveset:bigdecimal-2;REDMOND\jflam" Comment : BigDecimal contribution from Peter Bacon Darwin
I noticed the file name casing is wrong: bigdecimal.cs, fraction.cs. Was it broken by some transformation? Tomas -----Original Message----- From: Curt Hagenlocher Sent: Friday, October 03, 2008 4:03 PM To: John Lam (IRONRUBY); IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: bigdecimal-2 Code looks good. There might have been less churn to have CodeContext on each method now so that we can use it to get a Config later -- but there''s also something to be said for making it work first. Style consistency issues: The line endings in bigdecimal.cs appear to be a mix of LF and CRLF. They should all be CRLF. We tend to prefer not to say "this.varname = x"; if the symbol name conflicts, it should be changed if possible. -----Original Message----- From: John Lam (IRONRUBY) Sent: Friday, October 03, 2008 3:24 PM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: Code Review: bigdecimal-2 tfpt review "/shelveset:bigdecimal-2;REDMOND\jflam" Comment : BigDecimal contribution from Peter Bacon Darwin
One more note:
// TODO: This static field need to be added to a hash on
RubyExecutionContext
private static readonly BigDecimal.Config _config = new
BigDecimal.Config();
internal static BigDecimal.Config GetConfig() { return _config; }
There is already API on RubyContext for what you need:
public bool TryGetLibraryData(object key, out object value)
public object GetOrCreateLibraryData(object key, Func<object>
valueFactory)
public bool TryAddLibraryData(object key, object value, out object
actualValue)
Tomas
-----Original Message-----
From: Curt Hagenlocher
Sent: Friday, October 03, 2008 4:03 PM
To: John Lam (IRONRUBY); IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: bigdecimal-2
Code looks good. There might have been less churn to have CodeContext on each
method now so that we can use it to get a Config later -- but there''s
also something to be said for making it work first.
Style consistency issues:
The line endings in bigdecimal.cs appear to be a mix of LF and CRLF. They
should all be CRLF.
We tend to prefer not to say "this.varname = x"; if the symbol name
conflicts, it should be changed if possible.
-----Original Message-----
From: John Lam (IRONRUBY)
Sent: Friday, October 03, 2008 3:24 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: bigdecimal-2
tfpt review "/shelveset:bigdecimal-2;REDMOND\jflam"
Comment :
BigDecimal contribution from Peter Bacon Darwin
I''m going ahead and removing the static field. Love the
GetOrCreateLibraryData API! :)
Thanks,
-John
-----Original Message-----
From: Tomas Matousek
Sent: Friday, October 03, 2008 4:38 PM
To: Curt Hagenlocher; John Lam (IRONRUBY); IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: bigdecimal-2
One more note:
// TODO: This static field need to be added to a hash on
RubyExecutionContext
private static readonly BigDecimal.Config _config = new
BigDecimal.Config();
internal static BigDecimal.Config GetConfig() { return _config; }
There is already API on RubyContext for what you need:
public bool TryGetLibraryData(object key, out object value)
public object GetOrCreateLibraryData(object key, Func<object>
valueFactory)
public bool TryAddLibraryData(object key, object value, out object
actualValue)
Tomas
-----Original Message-----
From: Curt Hagenlocher
Sent: Friday, October 03, 2008 4:03 PM
To: John Lam (IRONRUBY); IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: bigdecimal-2
Code looks good. There might have been less churn to have CodeContext on each
method now so that we can use it to get a Config later -- but there''s
also something to be said for making it work first.
Style consistency issues:
The line endings in bigdecimal.cs appear to be a mix of LF and CRLF. They
should all be CRLF.
We tend to prefer not to say "this.varname = x"; if the symbol name
conflicts, it should be changed if possible.
-----Original Message-----
From: John Lam (IRONRUBY)
Sent: Friday, October 03, 2008 3:24 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: bigdecimal-2
tfpt review "/shelveset:bigdecimal-2;REDMOND\jflam"
Comment :
BigDecimal contribution from Peter Bacon Darwin