Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BATCH-1613 #3

Merged
merged 0 commits into from
Jul 18, 2011
Merged

BATCH-1613 #3

merged 0 commits into from
Jul 18, 2011

Conversation

snicoll
Copy link
Member

@snicoll snicoll commented Dec 24, 2010

Hi Dave,

Here's the patch in BATCH-1613.

HTH

@dsyer
Copy link
Member

dsyer commented Dec 27, 2010

Stephane, thanks for this. I'll add the multi threaded reader stuff when I get time to add some test cases (or you could provide some).

Note on the process: I like the fact that you flattened your changes down to a single commit. But, you also have two pull requests from the same branch, so it can get hard to distinguish. Better to offer pull requests from independent feature branches in future.

@snicoll
Copy link
Member Author

snicoll commented Dec 27, 2010

Yup, I am still new to git and I used the same clone for both requests. I will create a branch of my clone if I have further changes.

Regarding the patch, there are two things to improve:

  1. Easy: update a bit the contract of the base class so that I don't have to copy/paste some init code (I have added documentation where I did this). Having a protected getter would solve most of this
  2. The implementation is limited to one item = one row which will be an issue I guess. I don't see how we can workaround this limitation easily but if you know some, feel free and I'll review my patch.

@dsyer
Copy link
Member

dsyer commented Dec 30, 2010

Stephane, I don't have any record of you signing a contributor agreement (https://support.springsource.com/spring_committer_signup). Did you do that yet?

@snicoll
Copy link
Member Author

snicoll commented Dec 30, 2010

Hi Dave,

Just signed the agreement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants