Refactor to prepare for fixed-width column support. #219
Conversation
This commit should not introduce any behavioral changes.
kosak
force-pushed
the
kosak_refactor-prep
branch
from
80a5fa2 to
57c6b48
Compare
| * | ||
| * @param padding The padding byte. | ||
| */ | ||
| public void trimPadding(byte padding) { |
There was a problem hiding this comment.
This appears to be unused (even when reviewing other PR).
| /** | ||
| * Trim whitespace from the front and back of the slice. | ||
| * | ||
| * @param cs The slice, modified in-place to have whitespace (if any) removed. | ||
| */ | ||
| public static void trimWhitespace(final ByteSlice cs) { |
There was a problem hiding this comment.
This becoming newly public, I'm going to complain about the name "whitespace", as that has a more strict definition, see java.lang.Character#isWhitespace(char).
There was a problem hiding this comment.
Thanks. Renamed to trimSpacesAndTabs. PTAL
| import io.deephaven.csv.util.MutableInt; | ||
|
|
||
| public class ReaderUtil { | ||
| public static String[] makeSyntheticHeaders(int numHeaders) { |
There was a problem hiding this comment.
This can be moved / made private to the single callsite?
There was a problem hiding this comment.
Oops. There were supposed to be two call sites. Now there are :-). Thanks, good catch.
| public class DelimitedHeaderFinder { | ||
| /** | ||
| * Determine which headers to use. The result comes from either the first row of the file or the user-specified | ||
| * overrides. | ||
| */ | ||
| public static String[] determineHeadersToUse(final CsvSpecs specs, |
There was a problem hiding this comment.
This doesn't need to be public? Can be placed as package private class at same level as CsvReader?
There was a problem hiding this comment.
I moved a few files into new subpackages deephaven.csv.reading.cells and deephaven.csv.reading.headers
because I thought deephaven.csv.reading was getting a little crowded.
But, I realize Java is a little bit annoying by the way every directory is a subpackage, and one of the evils we have to live to avoid subpackages is we put a lot of .java files in the same directory.
At the end of the day these new subpackages are small. As of the second PR, there will be three files in deephaven.csv.reading.cells and two files in deephaven.csv.reading.headers
If you think it is more Java-canonical to move those files back up into the parent package. I can do that. Let me know.
There was a problem hiding this comment.
I agree, somewhat annoying the way packaging / visibility works in Java 8. Ultimately, my general preference is to reduce the "public API" to ensure we are free to change implementation details without breaking users. That said, deephaven-csv doesn't really follow this convention (lots of stuff that is "public API" that doesn't technically need to be), so I'm happy keeping this as-is.
If we bump to 9+, we can adopt modules which allows libraries to only export specific packages.
The purpose of this PR is to refactor the code in order to prepare for my next PR, which adds support for fixed-width columns.
This PR should not introduce any behavioral changes.
Partially resolves #212