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

Added methods to read excel properties from excel files #154

Merged
merged 6 commits into from
May 25, 2020
Merged

Added methods to read excel properties from excel files #154

merged 6 commits into from
May 25, 2020

Conversation

breucode
Copy link
Contributor

@breucode breucode commented May 6, 2020

This PR enables the possibility to read excel properties (but only from XLSX files!).

Solves #151

@breucode
Copy link
Contributor Author

breucode commented May 6, 2020

Currently missing:

  • Unit tests
  • Readme update
  • List of "core properties" (e.g. Properties, which excel has by default), which can be read
  • Proper JavaDocs

@breucode breucode changed the title Added methods to read excel properties from excel files - #151 Added methods to read excel properties from excel files May 6, 2020
@coveralls
Copy link

coveralls commented May 6, 2020

Coverage Status

Coverage decreased (-1.4%) to 91.184% when pulling c331a89 on breucode:master into 81f0ab2 on ozlerhakan:master.

@breucode breucode marked this pull request as ready for review May 6, 2020 14:20
@breucode breucode marked this pull request as draft May 6, 2020 14:21
@breucode breucode closed this May 6, 2020
@breucode breucode reopened this May 6, 2020
@breucode breucode marked this pull request as ready for review May 6, 2020 16:09
@ozlerhakan
Copy link
Owner

Hi @breucode , thanks for the changes! at first glance, we need to improve the code coverage a bit more :)

@breucode
Copy link
Contributor Author

Hi @breucode , thanks for the changes! at first glance, we need to improve the code coverage a bit more :)

Hi @ozlerhakan :). As far as I can see, the only new untested code are private constructors and exception handling.

Should we add testing for exception handling to Poiji? If we do so, we could add a mocking framework for non-static calls. However, a lot of calls are static (e.g. OPCPackage.open(...)), so we either need to find a parameter, which triggers the exception. Alternatively, we need to go a functional way, where we can switch real method with a method, which throws the needed exception.

What would you say @ozlerhakan ?

@ozlerhakan
Copy link
Owner

I think we need to refrain ourselves to use/create static methods.

@breucode
Copy link
Contributor Author

I think we need to refrain ourselves to use/create static methods.

I agree, but I don't see a way to replace calls like OPCPackage.open(...), which come from an external library.

@ozlerhakan
Copy link
Owner

I will look at it this weekend @breucode

@ozlerhakan ozlerhakan merged commit e059f4d into ozlerhakan:master May 25, 2020
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

3 participants