-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/** | ||
* File with alternate comment syntax. | ||
* This file uses double slash and regular star-slash comment styles for doc | ||
* strings. | ||
*/ | ||
|
||
syntax = "proto3"; | ||
|
||
// Message with | ||
// a | ||
// multi-line comment. | ||
message Test1 { | ||
|
||
/** | ||
* Field with a doc-block comment. | ||
*/ | ||
string field1 = 1; | ||
|
||
// Field with a single-line comment starting with two slashes. | ||
uint32 field2 = 2; | ||
|
||
/// Field with a single-line comment starting with three slashes. | ||
bool field3 = 3; | ||
|
||
/* Field with a single-line slash-star comment. */ | ||
bool field4 = 4; | ||
|
||
bool field5 = 5; // Field with a trailing single-line two-slash comment. | ||
|
||
bool field6 = 6; /// Field with a trailing single-line three-slash comment. | ||
|
||
bool field7 = 7; /* Field with a trailing single-line slash-star comment. */ | ||
|
||
bool field8 = 8; | ||
|
||
// Field with a | ||
// multi-line comment. | ||
bool field9 = 9; | ||
|
||
/** | ||
* Field with a | ||
* multi-line doc-block comment. | ||
*/ | ||
string field10 = 10; | ||
} | ||
|
||
/* Message | ||
with | ||
a multiline plain slash-star | ||
comment. | ||
*/ | ||
message Test2 { | ||
} | ||
|
||
/* | ||
* Message | ||
* with | ||
* a | ||
* comment and stars. | ||
*/ | ||
enum Test3 { | ||
|
||
/** Value with a comment. */ | ||
ONE = 1; | ||
|
||
// Value with a single-line comment. | ||
TWO = 2; | ||
|
||
/// Value with a triple-slash comment. | ||
THREE = 3; // ignored | ||
|
||
FOUR = 4; /// Other value with a comment. | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
var tape = require("tape"); | ||
|
||
var protobuf = require(".."); | ||
|
||
tape.test("proto comments in alternate-parse mode", function(test) { | ||
test.plan(17); | ||
var options = {alternateCommentMode: true}; | ||
var root = new protobuf.Root(); | ||
root.load("tests/data/comments-alternate-parse.proto", options, function(err, root) { | ||
if (err) | ||
throw test.fail(err.message); | ||
|
||
test.equal(root.lookup("Test1").comment, "Message with\na\nmulti-line comment.", "should parse double-slash multiline comment"); | ||
test.equal(root.lookup("Test2").comment, "Message\nwith\na multiline plain slash-star\ncomment.", "should parse slash-star multiline comment"); | ||
test.equal(root.lookup("Test3").comment, "Message\nwith\na\ncomment and stars.", "should parse doc-block multiline comment"); | ||
|
||
test.equal(root.lookup("Test1.field1").comment, "Field with a doc-block comment.", "should parse doc-block field comment"); | ||
test.equal(root.lookup("Test1.field2").comment, "Field with a single-line comment starting with two slashes.", "should parse double-slash field comment"); | ||
test.equal(root.lookup("Test1.field3").comment, "Field with a single-line comment starting with three slashes.", "should parse triple-slash field comment"); | ||
test.equal(root.lookup("Test1.field4").comment, "Field with a single-line slash-star comment.", "should parse single-line slash-star field comment"); | ||
test.equal(root.lookup("Test1.field5").comment, "Field with a trailing single-line two-slash comment.", "should parse trailing double-slash comment"); | ||
test.equal(root.lookup("Test1.field6").comment, "Field with a trailing single-line three-slash comment.", "should parse trailing triple-slash comment"); | ||
test.equal(root.lookup("Test1.field7").comment, "Field with a trailing single-line slash-star comment.", "should parse trailing slash-star comment"); | ||
test.equal(root.lookup("Test1.field8").comment, null, "should parse no comment"); | ||
test.equal(root.lookup("Test1.field9").comment, "Field with a\nmulti-line comment.", "should parse multiline double-slash field comment"); | ||
test.equal(root.lookup("Test1.field10").comment, "Field with a\nmulti-line doc-block comment.", "should parse multiline doc-block field comment"); | ||
|
||
test.equal(root.lookup("Test3").comments.ONE, "Value with a comment.", "should parse blocks for enum values"); | ||
test.equal(root.lookup("Test3").comments.TWO, "Value with a single-line comment.", "should parse double-slash comments for enum values"); | ||
test.equal(root.lookup("Test3").comments.THREE, "Value with a triple-slash comment.", "should parse lines for enum values and prefer on top over trailing"); | ||
test.equal(root.lookup("Test3").comments.FOUR, "Other value with a comment.", "should not confuse previous trailing comments with comments for the next field"); | ||
|
||
test.end(); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
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.There was a problem hiding this comment.
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.