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

Fix #102: Quote configurable delimiter character, not just commas #107

Merged
merged 6 commits into from
Aug 28, 2022
Merged

Fix #102: Quote configurable delimiter character, not just commas #107

merged 6 commits into from
Aug 28, 2022

Conversation

lardieri
Copy link
Contributor

In my initial commit, I'm keeping the visibility level of the new Serializer at "internal" so the API surface is equivalent to the previous code.

However, would you have any concerns about making Serializer be public? It might help me with a project I'm working on.

(Actually, what I really want is to add additional rows to an existing file. I'm still working out the design, so it's not clear yet whether I should append rows to my DataView and serialize the whole thing, or go directly to the file APIs to write out only the new rows.)

Copy link
Contributor

@DivineDominion DivineDominion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

The extraction of shared serialization logic makes sense, too. It introduces potential variance between the CSV's data and the serializer, because the serializer can be ab-used independently (see below). But in principle, it's a good idea IMHO!

However, would you have any concerns about making Serializer be public? It might help me with a project I'm working on.

(Actually, what I really want is to add additional rows to an existing file. I'm still working out the design, so it's not clear yet whether I should append rows to my DataView and serialize the whole thing, or go directly to the file APIs to write out only the new rows.)

In my Mac app TableFlip, I import from CSV to an internal tabular data structure, and export that to various formats.

I could see how the CSV type could be useful as a mutable table representation, but that's a bit out of scope for CSV -- that's a more generic "table" or a "tabular". (We could add that, too, but that needs more discussion I think.)

Producing CSV export data here does still make total sense, though, e.g. to quickly convert delimiters. Mutating the CSV output a bit sounds like a CSV-centric use case during export, too. It does make sense.

In the context of the SwiftCSV module, the name Serializer fits well. But when it becomes public API, should we call it CSVSerializer instead? Then again SwiftCSV.Serializer can be used to disambiguate, too. I have no strong opinion on this.

When we make Serializer public, we need handle more inconsistent data: it used to be the CSV views's job to assemble the serializable content in a way that made sense to the CSV view.

But if the serializer is extracted, and esp. if it's public, we can't make any assumptions.

We'd need

  • unit tests for all public functions, e.g. to demonstrate that the enquoting of cell contents works as expected;
  • handle non-rectangular data, i.e.:
    • what happens if some rows have less/more elements than others?
    • what happens if the header cell count is x, but a row's element count is >x?
    • I tend towards making header.count canonical; cut off row cells at offsets beyond header.count, and if rows are too short, fill with empty cells -- so that in effect, each row has the same amount of delimiters

This is a bit of extra work. Do you think that's worth it for your use case? If so, let's check out how to design this properly 💪

SwiftCSV/Serializer.swift Outdated Show resolved Hide resolved
SwiftCSV/Serializer.swift Outdated Show resolved Hide resolved
SwiftCSV/NamedCSVView.swift Outdated Show resolved Hide resolved
SwiftCSVTests/EnumeratedCSVViewTests.swift Outdated Show resolved Hide resolved
@lardieri
Copy link
Contributor Author

Let's keep the Serializer internal for now. You've raised many good points about appending or exporting rows, so I've moved that to a separate discussion.

Copy link
Contributor

@DivineDominion DivineDominion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! Looks good

@lardieri lardieri merged commit ada8ce6 into swiftcsv:master Aug 28, 2022
@lardieri lardieri deleted the enquote branch August 28, 2022 17:58
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.

None yet

2 participants