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

Allow quote & space characters to be configured #1

Closed
wants to merge 3 commits into from

Conversation

euank
Copy link
Owner

@euank euank commented Apr 6, 2016

Will this fit your needs, @LinuxMercedes?

Review welcome as well if you so desire

@LinuxMercedes
Copy link

I think this will be useful! I have a couple questions, though:

  1. What do you think about allowing backslash escaping of quoteCharacters, rawQuoteCharacters, and spaceChars? Should this be done instead of escapes for ', ", and , or in addition?
  2. (nit) ShouldspaceCharsbe namedspaceCharacters?

@euank
Copy link
Owner Author

euank commented Apr 7, 2016

Good call on both counts.. un momento

Also rename spaceChar to spaceCharacter to match the other naming
// Here we go, backslash escaping
// As a special case, always allow quoteCharacters, rawQuoteCharacters,
// and spaceCharacters to be escaped
var doubleQuoteChar = findElement(self.doubleQuoteCharacters, str[i]);

Choose a reason for hiding this comment

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

Does updating doubleQuoteChar here break quote matching? If doubleQuoteCharacters = ['"','$'], would this cause "\$" to fail with a missing close quote error?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nope, added a test showing that

@euank
Copy link
Owner Author

euank commented Nov 17, 2016

@LinuxMercedes I'm going through my old open PRs.

I assume you have no interest in this now so I should just decide myself if I want to close or merge it?

@LinuxMercedes
Copy link

Yeah, I ended up needing lower-level parser control for my usecase. Thanks for writing this, though!

@euank
Copy link
Owner Author

euank commented Jul 24, 2018

Abandoning this.

@euank euank closed this Jul 24, 2018
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.

2 participants