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

[IMAGING-239] Add inflate (deflate algorithm) to TIFF files #43

Closed
wants to merge 7 commits into from
Closed

[IMAGING-239] Add inflate (deflate algorithm) to TIFF files #43

wants to merge 7 commits into from

Conversation

pauldaustin
Copy link
Contributor

  • Adds inflate (deflate algorithm) to TIFF files.
  • Also allows passing params to reading directories, so that image data loading can be turned off
  • Make getTiffRawImageData public so that the image data can be loaded after the directories.

I was going to do 2 commits but then it got committed by accident.

@coveralls
Copy link

coveralls commented May 29, 2019

Coverage Status

Coverage increased (+0.002%) to 74.415% when pulling e655d2b on pauldaustin:feature/deflate into 1e725b0 on apache:master.

@kinow
Copy link
Member

kinow commented May 29, 2019

From an initial look the code looks good. @pauldaustin could you create a JIRA issue for it, and also include some basic unit tests, please? There's some info on contributing here. Thanks!

- Issue created https://issues.apache.org/jira/browse/IMAGING-226
- From TiffReader I removed one public method I'd added and changed the
other back to private.
- Added compression method to TiffRoundtripTest
@kinow
Copy link
Member

kinow commented May 31, 2019

Thanks for working on the PR comments so quickly. Could you take a look at the Travis CI broken build? It appears to be an issue with the code formatting.

You can look at the .travis.yml file, and run the same maven targets in your own computer to reproduce it.

@pauldaustin
Copy link
Contributor Author

Do you know if there are an exported configurations for Eclipse code style, formatting and import ordering. I use that on my own projects so it automatically gets formatted properly when I save.

I avoided using the eclipse auto formatter which would resolve these issues as it re-formatted a bunch of the other code. My code style also sorts methods into a consistent order which if run regularly on a project makes merging changes between branches easier.

@pauldaustin
Copy link
Contributor Author

Looks like the formatter wouldn't have helped as the spaces were at the end of lines in the javadoc comment and it didn't strip them when I tried the formatter.

@kinow
Copy link
Member

kinow commented Jun 1, 2019

Looks like the formatter wouldn't have helped as the spaces were at the end of lines in the javadoc comment and it didn't strip them when I tried the formatter.

👍 I kind of got used to the style used in most Commons projects, so I don't have an imported style/formatter settings for Eclipse (I also use it for Java). Some projects have mixed styles too, mixing old and new, with different rules. Easier to try to copy the existing style and run Maven targets before committing I guess.

New error:

[ERROR] 7002: org.apache.commons.imaging.common.bytesource.ByteSource: Method 'public java.lang.String getFilename()' has been removed

I think an existing method that was public has now been removed? That is not backward compatible - i.e. existing users would have issues unless we released a new major version (next release is still maintenance/alpha).

@garydgregory
Copy link
Member

I renamed getFilename() (obvious typo) to getFileName(). Since we do not have a 1.0 release yet, breaking BC is OK. We should get the public names right before 1.0.

@kinow
Copy link
Member

kinow commented Jun 1, 2019

I renamed getFilename() (obvious typo) to getFileName(). Since we do not have a 1.0 release yet, breaking BC is OK. We should get the public names right before 1.0.

Is there a way to tell cliir to ignore these errors for now then? So that the PR's checks can all pass

@pauldaustin
Copy link
Contributor Author

I merged master into my branch to get the other changes.

Maybe this will fix the PR check issues.

@kinow
Copy link
Member

kinow commented Jun 4, 2019

@pauldaustin no that won't fix the build. I've created a PR that disables Clirr check for backward compatibility - #45

If/once that gets merged, if you rebase your changes again then the build should pass this time.

Thanks
Bruno

@kinow
Copy link
Member

kinow commented Jun 8, 2019

Done @pauldaustin . If you rebase this PR onto master, the tests should finally pass. Sorry for the delay!

@pauldaustin
Copy link
Contributor Author

I'm giving up, seems to be a bunch of errors I'm not sure how to fix, or are in parts of the code I haven't changed.

@kinow
Copy link
Member

kinow commented Jun 25, 2019

@pauldaustin just had a look, and the PR is passing in some JDK's, and the failures appear to be related to Travis-CI. Nothing wrong with your code as far as I can tell, so I believe it's ready to be reviewed and hopefully merged :) 👍 thanks for rebasing it

@pauldaustin
Copy link
Contributor Author

@kinow That's great news thanks.

@kinow kinow changed the title Feature/deflate [IMAGING-239] Add inflate (deflate algorithm) to TIFF files Nov 10, 2019
@kinow kinow self-assigned this Nov 10, 2019
@kinow kinow closed this in 5e5483f Nov 11, 2019
@kinow
Copy link
Member

kinow commented Nov 11, 2019

Merged, thanks!

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