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

Restricting commentChar to ignore a line only if it appears at the start #662

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Restricting commentChar to ignore a line only if it appears at the start #662

merged 2 commits into from
Apr 30, 2020

Conversation

chrispomeroyhale
Copy link

Per our discussion in issue #254, I've built upon #624 to simplify the behavior we're expecting. Otherwise, there are different interpretations of the behavior a commentChar could have and there is varied support is across different CSV parsers. The approach here sets a baseline for what a comment should do at minimum which we can later extend with additional attributes if need be.

Is it worth bumping up the version number? In theory this breaks compatibility with the previous description of commentChar even though this updated definition is in line with the original motivation of adding this feature.

@rgieseke
Copy link
Contributor

rgieseke commented Mar 1, 2020

Looks good to me to have the simplified version, but i realizie that before and after it's not clear whether sequence or only a single-char are allowed.

@chrispomeroyhale
Copy link
Author

chrispomeroyhale commented Mar 2, 2020

Ah, good catch. I've tried to reword to better align with the verbiage of escapeChar and quoteChar. In the language I'm working with (Swift) a Character can potentially be more than one unicode values so this gets potentially confusing.

Does schemas/dictionary.json also need to be updated or does it derive information from schemas/dictionary/*.yml? I'm not too familiar with this project's setup.

One other thing: I imagine we could have a "row" ("entry"?) split over multiple lines, and have a comment within the row on a new line. So I've suggested here that we only skip if the comment char is at the beginning of a row not just a line. Is this reasonable?

@rgieseke
Copy link
Contributor

rgieseke commented Mar 2, 2020

Does schemas/dictionary.json also need to be updated or does it derive information from schemas/dictionary/*.yml? I'm not too familiar with this project's setup.

Looks like that's generated here:
https://github.com/frictionlessdata/specs/blob/master/build.js#L43

@rufuspollock
Copy link
Contributor

LGTM so I'm merging.

Great work @chrispomeroyhale and @rgieseke - great to see this clarification in.

For the record I would like a version bump (maybe in separate PR) but strictly i think this is ok b/c i think this was always the intended behaviour.

@rufuspollock rufuspollock merged commit 4312037 into frictionlessdata:master Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants