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

Support multiple comment symbols #766

Closed
wants to merge 2 commits into from

Conversation

@zeehio
Copy link
Contributor

@zeehio zeehio commented Dec 17, 2017

This pull request allows to provide more than one comment pattern to readr. Example:

readr::read_delim(c(
  "# A comment",
  "x,y",
  "1,2",
  "// another comment",
  "3,4"), delim = ",", comment = c("#", "//"))

It is applied on top of this other PR #677, so merging this will be easier once that PR is reviewed.

zeehio added 2 commits Dec 17, 2017
If this encoding is ambiguous in endianness (like UTF-16 or UTF-32 which mandate
a Byte Order Mark) the BOM is detected and skipped (as before) and the encoding
is updated to reflect the endianness (UTF-16LE, UTF16-BE...)
This commit allows users to define multiple comments using a character vector.

Additionally, comment detection is now handled exclusively by the datasource, before it was splitted between both the datasource and the tokenizer.

The `comment` argument in the tokenizer functions is deprecated and will be removed in future versions.
@zeehio zeehio force-pushed the zeehio:multiple_comment_prefixes branch from 828d243 to b5760c3 Dec 17, 2017
@jimhester
Copy link
Member

@jimhester jimhester commented Sep 11, 2020

I am reposting my comment from #677 here, again I sincerely apologize but I don't see another option and I don't think it is fair to keep these PRs open when there is no real chance of them being merged.

First, I wanted to thank you for working on this PR and the previous ones. This was not a simple problem and I appreciate the effort you went through to solve it.

I also wanted to apologize for leaving this open for so long, it was certainly not my intent to keep you waiting, but other projects and priorities kept me busy.

We have plans to begin moving away from the legacy parser in readr, in favor of the parsers in vroom or possibly arrow. Because of this I don't think it would be prudent to add this amount of new code to the existing parser.

Developing good software requires relentless focus, which means that we have to say no to many good ideas.

Even though we're closing this issue, we really appreciate your work, and hope you'll continue to contribute in the future.

@jimhester jimhester closed this Sep 11, 2020
@zeehio
Copy link
Contributor Author

@zeehio zeehio commented Sep 11, 2020

Hi @jimhester, I totally understand the situation. For me these PR were part of a learning process and it was a wonderful experience to interact with you and the rest of the team. It was worth it, even if in the end it never got merged. I understand the advantages of vroom (using ALTREP, etc) and I'm looking forward to keep contributing in the future as I've done in the past.

Besides, RStudio has my CV in case you ever decide to eventually hire more people and find me worth considering (it's a long shot but hey, it's always worth trying 😅)

Thanks for your time and nice words!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants