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 restricting "lineCommentStyle" match to the beginning of a line. #16

Closed
jgerigmeyer opened this issue Dec 5, 2017 · 12 comments
Closed

Comments

@jgerigmeyer
Copy link
Contributor

jgerigmeyer commented Dec 5, 2017

As its written now, any occurrence of the lineCommentStyle will be matched and stripped from the parsed return value. This makes it impossible to "comment a comment", or use the lineCommentStyle string within a comment. For example, using SassDoc, it would be nice to be able to have the comment syntax within a comment (to show how to make a comment):

/// Here is how to add a comment:
/// /// I am a comment

Right now, the regex matches every occurrence of ///, resulting in lines:

['Here is how to add a comment:', 'I am a comment']

Instead of what I'd like:

['Here is how to add a comment:', '/// I am a comment']

One "fix" would be enforcing that comments must begin with a newline by changing the regex in index.js#L80 from:

new RegExp(escapeStringRegexp(lineCommentStyle) + '[\\/]*');

to:

new RegExp('(^|\n)' + escapeStringRegexp(lineCommentStyle) + '[\\/]*');

I'm not sure if that would break other use-cases (it doesn't break anything else in my SassDoc project, at least)... Perhaps it could be an option turned on or off?

Or even better, is there a way to mark a section of text as "escaped" such that it won't be stripped by CDocParser, or enforce only one comment per line?

@jgerigmeyer
Copy link
Contributor Author

I submitted a PR (#17) which addresses this without breaking other use-cases. If two or more instances of lineCommentStyle occur on the same line, it does not try to interpret any of them as comments after the first instance.

@pascalduez
Copy link
Collaborator

Hi,

it feels reasonable to only allow one doc comment per line, at least in the SassDoc context.

As for this PR I hope @FWeinb will be able to have a look at it.
Otherwise we might have to fork CDocParser to the SassDoc organisation and start using it...

@FWeinb
Copy link
Owner

FWeinb commented Dec 22, 2017

Sorry for not responding earlier. I added @pascalduez as a maintainer to cdocparser. I would be glad if you could merge the PR.

@jgerigmeyer
Copy link
Contributor Author

@pascalduez Anything holding up merging the PR, etc? Thanks!

@pascalduez
Copy link
Collaborator

@FWeinb Do I have publish rights on npm as well? ;)

@FWeinb
Copy link
Owner

FWeinb commented Jan 8, 2018

@pascalduez Sorry for that. Just added you.

@pascalduez
Copy link
Collaborator

pascalduez commented Jan 9, 2018

@jgerigmeyer [email protected] published.

I will need to run some tests before bumping the dependency in scss-comment-parser, there was a 0.14.0 version which we haven't yet finished integrating, and I'm wondering if there's some breakages. See SassDoc/sassdoc#410 and d795b1f

@jgerigmeyer
Copy link
Contributor Author

@pascalduez What breakages are you seeing? There's a failed build on that PR merge (https://travis-ci.org/SassDoc/sassdoc/builds/90808006), but it looks like it's a simple linting error. Is there something else I'm missing?

@pascalduez
Copy link
Collaborator

@jgerigmeyer That branch was never merged into master, and I don't want to rush releasing this, it would need proper docs etc.

@jgerigmeyer
Copy link
Contributor Author

@pascalduez That makes sense... though I'll point out that since scss-comment-parser is requiring "cdocparser": "^0.13.0", most people will already be getting v0.14.0 (and now v0.15.0), automatically, and have been for awhile. https://github.com/SassDoc/scss-comment-parser/blob/master/package.json#L17

@pascalduez
Copy link
Collaborator

@jgerigmeyer I though 0.*.* based semver ranges does not behave the same and as such should stick to 0.13. Testing...

@jgerigmeyer
Copy link
Contributor Author

jgerigmeyer commented Jan 10, 2018

@pascalduez Oh, you're correct. My mistake! (I've been using v0.14.0 -- and now v0.15.0 --
without issue, for what it's worth.)

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

No branches or pull requests

3 participants