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

#4149 - FindBugs fixes #3

Merged
merged 18 commits into from
Feb 16, 2012
Merged

Conversation

melissalinkert
Copy link
Member

Resolves all bugs/warnings found by FindBugs, and updates the 'findbugs' Ant target to fail if a bug or warning is found.

Should close #4149.

This allows it to find absolutely all of the classes that it needs.
Usually, it is just the slf4j/log4j JARs that are needed (and not
included in the compile-time classpath).
No component generates loci-io-image.jar, so it shouldn't be on the classpath.
This prevents FindBugs from failing because it can't locate loci-io-image.jar.
Noticed by FindBugs.
...since we also overrode Object.equals(Object).  Noticed by FindBugs.
@joshmoore
Copy link
Member

The only thing I can see is some remaining, non-constant UTF-8s. @chris-allan, did you get notified of this PR?

...so now we are not writing "UTF-8" as a non-constant in many different places.
@melissalinkert
Copy link
Member Author

Non-constant usages of "UTF-8" should be resolved now. The default encoding is now defined once in a new class (loci.common.Constants) and used across all of the components.

@chris-allan
Copy link
Member

@joshmoore: Sure did. For what it's worth explicitly setting a "UTF-8" encoding on any of these transforms is bound to potentially introduce or fix several things. It is not the default encoding on Mac OS X for instance.

@melissalinkert
Copy link
Member Author

Fair enough. UTF-8 is the default encoding on Linux (at least for me), though, so I'd be a little surprised if it causes major problems. If you would rather not set encoding to UTF-8 across the board, then we'll need to convince FindBugs not to complain when the default encoding is used.

@joshmoore
Copy link
Member

Doh. Waited so long that this no automatically merges. Might have to take care of manually. Was there any decision on the UTF-8 front?

@melissalinkert
Copy link
Member Author

@joshmoore @chris-allan and I talked about it last week, and the decision was that I need to write some tests to compare UTF-8, MacRoman, and Windows-1252 encodings (the default encodings on various platforms) before we go further.

If it's easier, I can close this PR, fix up the branch so that it can be automatically merged, and then re-open once tests are done and pushed.

@joshmoore
Copy link
Member

Hold off on the closing/re-opening, Melissa. Chris suggested a better way to take care of those that we can try out when you are ready.

These tests will fail on all of the test strings that contain non-ASCII
characters.
@melissalinkert
Copy link
Member Author

Encoding tests added in above commit. Enforcing UTF-8 across the board will mean that any files encoded with MacRoman or Windows-1252 will be decoded incorrectly if any non-ASCII characters are present. But then, we would have had this problem anyway when reading a MacRoman file on anything other than Mac OS 9 or when reading a Windows-1252 file on anything other than Windows.

@chris-allan
Copy link
Member

@melissalinkert: This one also needs the same develop merge conflict resolution as pr-9. Otherwise I think we're just going to have to document this change heavily and accept that we may have to fix some regressions.

Conflicts:
	components/bio-formats/src/loci/formats/in/NativeND2Reader.java
chris-allan added a commit that referenced this pull request Feb 16, 2012
@chris-allan chris-allan merged commit 2de4006 into ome:develop Feb 16, 2012
@ctrueden ctrueden mentioned this pull request Apr 14, 2012
joshmoore pushed a commit that referenced this pull request Nov 5, 2012
joshmoore added a commit to snoopycrimecop/bioformats that referenced this pull request Feb 6, 2014
Fix test suite's logback.xml to write to a logfile
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Mar 20, 2014
Remaining fixes:

For small wavelengths and light source linking
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Aug 6, 2014
Add intermediate ReaderTest class to initialize reader and default sizes
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Mar 18, 2015
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Mar 24, 2015
Remove unnecessary image count check
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Apr 15, 2015
Fix formatting for list, note & menu options
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Dec 12, 2018
Add test to check that explicitly disabling tiling works
snoopycrimecop pushed a commit to snoopycrimecop/bioformats that referenced this pull request Jun 10, 2019
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.

3 participants