Simplify support other file formats, e.g. Xslx

[hallvard]

I'm looking into supporting Excel files (xslx) by using the Apache POI project. I would like to reuse some of the logic from the corresponding csv code, like CsvReadOptions and its Builder class, but that's pretty difficult. First, the CsvReadOptions has a private constructor (called by its Builder), so you cannot subclass it. Second, some options don't make sense for a format that's not line-oriented, e.g. separator and lineEnding. In addition, the customParser method in ColumnType takes an CsvReadOptions instance as argument, so it needs to be subclassed.

I propose making an AbstractReadOptions class that includes the things that other formats are most likely to need, and that is easy to subclass. And update the customParser method in ColumnType to use that class as argument instead.

[benmccann]

It sounds to me like you're looking at an old version of the code because we recently made some of these changes to support adding the JSON and fixed-width file readers. Can you sync your repo against the latest code from master?

[hallvard]

Yes, you're right! It seems like the kind of changes I suggested are in the newest version. My plan is to create a new maven module with the XslxReader and XslxReadOptions in a tech.tablesaw.io.xslx package, so it needn't be part of the core. Then integrate it into the build and create a pull request. Sounds OK? Should I create a separate issue first?

[benmccann]

No need to create a separate issue

I'm not sure the best API for optional dependencies would be or how to best include them. We should probably follow the same pattern for the JSON, HTML, and CSV dependencies as well.

For including, we have the Table.smile() optional dependency that makes use of Maven's optional attribute. There's also the option to make a separate Maven dependency as you suggested.

For API, most stuff is done today via Table.read().csv(...), Table.read().json(...), Table.read().html(...), etc. If we have a separate Maven module, it'll have to be a bit different and we'll need to do something like new XslxReader().read(...). I'm curious to learn more about how Spark handles this because I think their API is similar to ours, but they have the ability to register new readers as plugins. I'm not quite sure how that works, but I'd be interested to learn more before we make a decision

[benmccann]

I think I'd probably lean towards including the POI dependency as an optional dependency like the Smile dependency and creating the reader in the main module alongside the json, csv, html readers

[lwhite1]

I agree

…

On Thu, Feb 21, 2019 at 11:02 PM Ben McCann ***@***.***> wrote: I think I'd probably lean towards including the POI dependency as an optional dependency like the Smile dependency and creating the reader in the main module alongside the json, csv, html readers — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#466 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADRXggNE6_JEAujfm4gCy_KoSDiebTTKks5vP2vsgaJpZM4bE3fp> .