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

Adds a new option to IParseOptions, alternateComment. #968

Merged
merged 2 commits into from
Feb 6, 2018

Conversation

gideongoodwin
Copy link
Contributor

If this new flag is true, the tokenizer will change the way it parses comments from the input. Instead of requiring /// or /** / (docblock style) comments only, it will additionally accept // and / */ comments. It will consecutive lines of // comments into one multi-line comment.

The motivation here is that we want to parse comments in customer api descriptions which don't adhere to the docblock style.

…his activates an alternate comment parsing mode that preserves double-slash comments.
src/tokenize.js Outdated
// standard comment parsing
if (charAt(offset) === "/") { // Line
// check for triple-slash comment
isDoc = charAt(start = offset + 1) === "/";
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of doing something like

isDoc = (charAt(start = offset) === "/" && ++offset) || alternateCommentMode;

here so it always handles /// and advances properly, and otherwise falls back to // in alternateCommentMode? Just asking because it seems that duplicating this section could potentially be avoided.

I'd say it would be fine if /// in non-alternateCommentMode also coalesces consecutive lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking. I agree that avoiding duplication would be good, however I am hesitant to change existing behavior in case other consumers of this library depend on it.

In the spirit of your comment, I consolidated the /* handling for original and alternate parsing modes. The ////// handling is a bit too divergent to consolidate further.

@dcodeIO dcodeIO merged commit 8400f87 into protobufjs:master Feb 6, 2018
@dcodeIO
Copy link
Member

dcodeIO commented Feb 6, 2018

Looking good, thank you!

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