+1. I read over most of the patch and see three distinct patterns:
- replacing ad-hoc string arguments that name file paths with Path objects
- replacing ad-hoc temp file allocation, deallocation with a uniform
mechanism
- whitespace formatting differences between your and my formatters
Kudos for doing this and do you have an Eclipse formatter that handles
method argument indenting better than mine (std Lucene AFAICT)?
Jeff
PS: I share Ted's grumpus but, mea culpa, sometimes I introduce them for
debugging and don't inline them when I'm done. I do like them in other
situations where they make the code clearer in complicated argument
lists as in x.foo(y.bar().baz(), z.yak().fee().fie().foe()), but
probably Ted likes them in those situations too.
On 5/4/10 5:32 PM, Ted Dunning (JIRA) wrote:
[
https://issues.apache.org/jira/browse/MAHOUT-302?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12864069#action_12864069
]
Ted Dunning commented on MAHOUT-302:
------------------------------------
I read through about 20% of the patch. For this kind of patch, I am going to
assume that is a fair sample.
In general, I like this kind of change. The closest I came to grumping was
cases of a variable being declared to be used only in the next line where the
variable name didn't actually add anything new. I tend to prefer in-lines.
The fact that my only grumpus was that minor is a good sign, I think.
Change tests to use temp directories instead of output, testdata
----------------------------------------------------------------
Key: MAHOUT-302
URL: https://issues.apache.org/jira/browse/MAHOUT-302
Project: Mahout
Issue Type: Task
Components: Classification, Clustering, Collaborative Filtering,
Frequent Itemset/Association Rule Mining, Genetic Algorithms, Math, Utils
Affects Versions: 0.4
Reporter: Robin Anil
Assignee: Sean Owen
Fix For: 0.4
Attachments: MAHOUT-302.patch
