ftcsv icon indicating copy to clipboard operation
ftcsv copied to clipboard

Optional delimiter

Open timothymtorres opened this issue 2 years ago • 4 comments

Features

  • The delimiter is now an optional argument that defaults to the comma character , (since ya know, this is a CSV library)
  • The delimiter has been moved to the options table in case the user wants to change it
  • All unit tests have been updated to accommodate the new changes

Closes #23

timothymtorres avatar Mar 21 '23 05:03 timothymtorres

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.

FourierTransformer avatar May 31 '23 01:05 FourierTransformer

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?

timothymtorres avatar May 31 '23 02:05 timothymtorres

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 avatar May 31 '23 03:05 FourierTransformer

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 avatar May 31 '23 03:05 FourierTransformer

closing in favor of #42

FourierTransformer avatar Aug 05 '24 21:08 FourierTransformer