Re: [Rails-devel] Push Request : 1880 Code Update Part II

[brett l. <bre...@gm...>]

I think it's because the patches are missing any mail header
information. "am" stands for "apply mailbox", which makes it
especially useful if you use a text-based mail program, like mutt or
pine.

"git apply" (which is what 'git am' calls) does seem to correctly
detect the patch.

---Brett.



On Tue, Jan 24, 2012 at 5:06 PM, Erik Vos <eri...@xs...> wrote:
> Thanks Martin, this is much better!
>
>
>
> However, I’m having a problem to load these patches with ‘git am’, it says
> “Patch format detection fails”.  Does anyone have a clue?
>
>
>
> Erik.
>
>
>
> From: Dr....@t-... [mailto:Dr....@t-...]
> Sent: Tuesday, January 24, 2012 8:30 PM
> To: Rails Development
> Subject: [Rails-devel] Push Request : 1880 Code Update Part II
>
>
>
> Hi Eric, et al.
>
>
>
> please find attached the Code changes based on the originally submitted
> patch on Sunday.
>
>
>
> Alternativly i have also attached the whole patch against the master also.
>
>
>
> 1.       Patch line 291: please don’t use static variables to represent
> dynamic and game-instance dependent data.  We want to be prepared to process
> multiple game instances in one program instance.
>
> Fixed temporarly as i still need to find out how to access the instance of
> gameManager to get the variable to show (Lack of Java Experience on my side)
>
>
>
> 2.       Patch line 341: can’t the constant ‘CAPITALISE_FIFTY_PERCENT’
> better be replaced by the more generic ‘CAPITALISE_PERCENTAGE’ with an
> additional numeric attribute that states the percentage?
>
>
>
> Took out that code entirely. Hardcoded the capitalisation in floatCompany of
> StartRound_1880 and StockRound_1880.
>
>
>
> 3.       I really wonder why you think you need an extra method
> PublicCompanyI.getNumberOfTileLays() with an additional company argument.  A
> company knows its own identity and name, doesn’t it?  So why pass that
> identity another time?
>
>
>
> Lack of Java Experience, understood now :)
>
>
>
> 4.       Patch lines 416-421:  you shouldn’t include 1880-specific code into
> the generic  Round class the way you do here.  I don’t want to see
> references to 1880 and Shanghai in this class.   If you can’t find a way to
> reformulate this in a generic way, you can always override floatCompany()
> in  StockRound_1880.
>
>
>
> See 2.
>
>
>
> 5.       Patch lines 644-648:  similar comment as point 2.  I believe we
> have discussed this before.
>
>
>
> Fixed.
>
>
>
> 6.       BuyStartItem: why add a new boolean method isSharePriceToSet() if
> we already have the identical hasSharePriceToSet()?  And why can’t the whole
> Building Rights code be included in BuyStartItem_1880, which you have
> created anyway, instead of polluting BuyStartItem with that?
>
>
>
> Incidentally that was a left over as the code had already been brought down
> to BuyStartItem_1880 and stemmed from some refactoring attempts that didnt
> get cleaned up, sorry about that.
>
>
>
> 7.       Similarly, why can’t the whole par slots code be included into
> StockMarket_1880?
>
>
>
> Will be moved there.
>
>
>
> And thanks again for your comments Erik :)
>
>
>
> Kind Regards,
>
>
>
> Martin
>
>
>
>
>
>
> ------------------------------------------------------------------------------
> Keep Your Developer Skills Current with LearnDevNow!
> The most comprehensive online learning library for Microsoft developers
> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
> Metro Style Apps, more. Free future releases when you subscribe now!
> http://p.sf.net/sfu/learndevnow-d2d
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>