FourierTransformer / ftcsv

a fast csv library written in pure Lua
MIT License
73 stars 20 forks source link

Optional delimiter #37

Closed timothymtorres closed 3 months ago

timothymtorres commented 1 year ago

Features

Closes #23

FourierTransformer commented 1 year ago

Thanks for the PR!

I was thinking about this a little, instead of cutting a hard v2 that will break versions if people upgrade (or if they don't specify versions in their luarocks file). There's a way to support parse(fileName, delimiter, options) and parse(fileName, options) by checking the type of the second argument. I might play around with that idea a little before merging this in.

timothymtorres commented 1 year ago

I was under the impression it didn't need to be backwards compatible since you marked it with a v2 label. Another option is I can make it throw an error when a user attempts to use the old argument type and include a deprecated version message.

Which way do you prefer?

FourierTransformer commented 1 year ago

Yeah, I put that label up many years back, and never quite realized it could be done in a backwards compatible way.

I'm testing out this method that checks the types of the args, and returns the delimiter and options that can then get used.

local function determineArgumentOrder(delimiter, options)
    -- backwards compatibile layer
    if type(delimiter) == "string" then
        return delimiter, options

    -- the new format for parseLine
    elseif type(delimiter) == "table" then
        local realDelimiter = delimiter.delimiter or ","
        return realDelimiter, delimiter

    -- if nothing is specified, assume "," delimited and call it a day!
    else
        return ",", nil
    end
end

running into some weirdness when calling parse multiple times in a row right now

FourierTransformer commented 1 year ago

huh, nevermind. That seems to be unrelated. I'll create a new issue for it. I checked the code I was playing with in https://github.com/FourierTransformer/ftcsv/commit/e1d58dfbfc9a8eb660c3f2ad8be39223206c049c

FourierTransformer commented 3 months ago

closing in favor of #42