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-2041: Add support for reading excel files #2

Closed
wants to merge 49 commits into from
Closed

BATCH-2041: Add support for reading excel files #2

wants to merge 49 commits into from

Conversation

mdeinum
Copy link
Collaborator

@mdeinum mdeinum commented Mar 24, 2014

This commit adds support for reading Excel files. Both JExcel and Apache POI are supported. Support for reading Excel files has been designed similar to the support for reading flat files.

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.


<groupId>org.springframewor.batch</groupId>
<artifactId>spring-batch-excel</artifactId>
<version>1.3.0.BUILD-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 0.5.0.BUILD-SNAPSHOT or something along those lines. Since we don't have a release yet, it shouldn't be above 1.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Initially left the versions we used internally, now it is a 0.5.0.BUILD-SNAPSHOT.

@mminella
Copy link
Member

mminella commented Apr 1, 2014

Overall, this looks like a solid start. Beyond the line specific comments above, some more general comments:

  1. Version numbers - Since this is the initial release of all of this code, I'd expect them all to be in sync.
  2. Copyright date - Same as version numbers. I'd expect all the dates to be the same (and be either just 2014 or 2013-2014).
  3. @Override for implementing interfaces - Can we get @Override applied to methods that implement interface methods?

JUnit 4.12
Mockito 1.10.19
Fixed scope of Spring-test dependency
Removed the doCloseWorkbook method in favor of the normal doClose method.

The PoiItemReader now closes the InputStream and Workbook (for Apache POI >= 3.11)
Remove <p> and </p> where they didn't match.
Fixed javadoc errors and warnings
Added package-info files.
inline numberOfRows, numberOfColumns and the name for performance.
Fixed typo
Cache FormulaEvaluator instead of creating a new one.
Use the BeanWrapper to bind values to beans. Based on the BeanWrapperFieldSetMapper.
Added Java Config
Added BeanWrapperRowMapper
Numeric type can contain a Date instead of a number, check
added for Date and return the time in millis if a Date is found.
@dusiema
Copy link

dusiema commented Mar 4, 2015

Will this be merged any time soon? What is the current status of this?

@mminella
Copy link
Member

mminella commented Mar 4, 2015

Unless you have any objections, I'm going to merge this with one small update which is to update the @Depricated notes to point to the POI implementation (so users know what to use instead).

@dusiema
Copy link

dusiema commented Mar 4, 2015

No, I don't have any. I am using this already as a clone from https://github.com/mdeinum/spring-batch-extensions. While using it I found one or two things that could be improved (like being able to specify just one sheet in an excel file). But I think this could also be done, when it is integrated into this repo.

@mdeinum
Copy link
Collaborator Author

mdeinum commented Mar 5, 2015

@mminella Would this be worth a blog post explaining things a little?

@dusiema Maybe you could create an issue here at github and provide a pull request if you already have the code (after it has been merged ofcourse).

@dusiema
Copy link

dusiema commented Mar 5, 2015

@mdeinum: I don't have the code yet (I just deleted all the other sheets from the excel file). I will create an enhancement issue and mabye I can also provide a pull request later on. However before creating the issue in this repo I guess the code should be here first.

@dusiema
Copy link

dusiema commented Mar 11, 2015

@mminella: Any update on the integration? I have two more enhancement suggestions and I could provide pull-requests for them. I just thought I could provide them right here instead of the repo from @mdeinum.

@mdeinum
Copy link
Collaborator Author

mdeinum commented Mar 11, 2015

@dusiema If you have the enhancements you can create 1 or 2 pull requests to my repo. I'll merge them and then they should show up here also.

@dusiema
Copy link

dusiema commented Mar 11, 2015

@mdeinum I already thought about that. Need to get back to my computer at work. I'll try to do it tomorrow.

@mminella
Copy link
Member

Hey guys, I'm at a conference through tomorrow. I plan on merging this Friday.

@dusiema
Copy link

dusiema commented Mar 11, 2015

@mminella Cool.
I'll try to do the pull request anyways so @mdeinum can have a look first.

@mminella mminella assigned mminella and lucaslward and unassigned mminella Mar 13, 2015
@mminella
Copy link
Member

Merged. Thanks for all the hard work!

@mminella mminella closed this Mar 13, 2015
@mminella mminella assigned mminella and unassigned lucaslward Mar 13, 2015
@mdeinum
Copy link
Collaborator Author

mdeinum commented Mar 13, 2015

Great... Thanks for merging it. I'll try to write a blog post in the next couple of days.

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.

None yet

8 participants