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

Optional delimiter #37

Conversation

timothymtorres
Copy link

@timothymtorres timothymtorres commented Mar 21, 2023

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

The default behavior should always be assumed to be a comma since this is a CSV tool.
@FourierTransformer
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

huh, nevermind. That seems to be unrelated. I'll create a new issue for it. I checked the code I was playing with in e1d58df

@FourierTransformer
Copy link
Owner

closing in favor of #42

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.

Optional Delimiter
2 participants