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

Dropping C-style comments #326

Closed
HugoGiraudel opened this Issue Jan 19, 2015 · 28 comments

Comments

Projects
None yet
7 participants
@HugoGiraudel
Member

HugoGiraudel commented Jan 19, 2015

We initially supported C-style comments because I wrote the very first version of the parser and it was the easiest thing to parse for me since I am a big old blockhead. Then we allowed inline comments because libraries using SassDoc such as SassyLists output C-style comments, polluting the user's CSS. This was especially relevant in online playgrounds such as SassMeister.

I cannot find a good reason to use C-style over inline comments for SassDoc. They are longer and likely to be confusing in regard to usual CSS comments. Also, there are case where C-style comments simply fail:

/**
 * The CSS selector for a special button.
 *
 * @example
 *   #{$special-button-selector} {
 *     font-weight: bold;
 *   }
 * 
 */
$special-button-selector: '.button.special-button';

Undefined variable: "$special-button-selector".

Also it might be worth nothing that both CSS Guidelines and Sass Guidelines recommend using inline comments when documenting Sass code. It makes sense. C-style is for CSS, inline for Sass.

Back to my initial idea: what if we dropped C-style comments altogether? Having a single type of documentation is enough and less confusing for the end user. Also, allowing C-style comments for SassDoc might encourage library authors to pollute users' CSS with massive comment blocks.

@SassDoc/owners What do you think?

PS: this is now or never. API break.

@HugoGiraudel HugoGiraudel added this to the 2.0 milestone Jan 19, 2015

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Jan 19, 2015

The main problem with using block comments for documentation is that it pollutes the resulting CSS. If a library uses block comments, then the CSS of all websites using that library will be bloated with the libraries documentation.

I think you should keep supporting block comments forever, but you should edit all official examples to use line comments and warn library authors against using block comments (both in documentation and in SassDoc console output).

lolmaus commented Jan 19, 2015

The main problem with using block comments for documentation is that it pollutes the resulting CSS. If a library uses block comments, then the CSS of all websites using that library will be bloated with the libraries documentation.

I think you should keep supporting block comments forever, but you should edit all official examples to use line comments and warn library authors against using block comments (both in documentation and in SassDoc console output).

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jan 19, 2015

Member

So far I can't find any good use case for C-style comments neither.
Especially because SassDoc only support Sass items, that would never appears in the resulting CSS. So they don't need to carry their comments with them.

I'm fine with dropping them, as long as it won't involves huge refactor for v2.0.
Actually it should rather be at the CDocParser level ?

If we keep them, then indeed, we should forewarn in the docs.

Member

pascalduez commented Jan 19, 2015

So far I can't find any good use case for C-style comments neither.
Especially because SassDoc only support Sass items, that would never appears in the resulting CSS. So they don't need to carry their comments with them.

I'm fine with dropping them, as long as it won't involves huge refactor for v2.0.
Actually it should rather be at the CDocParser level ?

If we keep them, then indeed, we should forewarn in the docs.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

I'm fine with dropping them, as long as it won't involves huge refactor for v2.0.

👍

And I don't think it will affect SassDoc at all, it's just about scss-comment-parser.

If we keep them, then indeed, we should forewarn in the docs.

👍

Member

valeriangalliat commented Jan 19, 2015

I'm fine with dropping them, as long as it won't involves huge refactor for v2.0.

👍

And I don't think it will affect SassDoc at all, it's just about scss-comment-parser.

If we keep them, then indeed, we should forewarn in the docs.

👍

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jan 19, 2015

Member

And I don't think it will affect SassDoc at all, it's just about scss-comment-parser.

Whow okay, easy-peasy-lemon-squeezy then ?

Member

pascalduez commented Jan 19, 2015

And I don't think it will affect SassDoc at all, it's just about scss-comment-parser.

Whow okay, easy-peasy-lemon-squeezy then ?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

Well my link was wrong, it was just about the code extraction, but not the comments matching/parsing.

A simple way to disable C-style comments was to pass something that will never match as blockCommentStyle to CDocParser.CommentExtractor, so in scss-comment-parser:

var extractor = new CDocParser.CommentExtractor(scssContextParser, {
  blockCommentStyle: 'NOPE',
});

This is working fine for me, but we should be able to pass something like false to disable it at all, we'll see with @FWeinb to add this support in CDocParser.

Member

valeriangalliat commented Jan 19, 2015

Well my link was wrong, it was just about the code extraction, but not the comments matching/parsing.

A simple way to disable C-style comments was to pass something that will never match as blockCommentStyle to CDocParser.CommentExtractor, so in scss-comment-parser:

var extractor = new CDocParser.CommentExtractor(scssContextParser, {
  blockCommentStyle: 'NOPE',
});

This is working fine for me, but we should be able to pass something like false to disable it at all, we'll see with @FWeinb to add this support in CDocParser.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

Can someone write the best Regex possible to upgrade from C-Style to inline comments? I know, this is hard.

Member

HugoGiraudel commented Jan 19, 2015

Can someone write the best Regex possible to upgrade from C-Style to inline comments? I know, this is hard.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

Well, we could say:

  • s,^/\*\*,///, (replace lines that begin with /** by ///).
  • s,^ *\*/,///, (replace lines that begin with potential space plus */ by ///).
  • s,^ *\*,///, (replace lines that begin with potential space plus * by ///).

So:

find . -name '*.s[ac]ss' -exec sed -i '' 's,^/\*\*,///,;s,^ *\*/,///,;s,^ *\*,///,' {} + # BSD/Mac `sed`
find . -name '*.s[ac]ss' -exec sed -i 's,^/\*\*,///,;s,^ *\*/,///,;s,^ *\*,///,' {} + # GNU `sed`

But by all means, this is regex, this is not parser.

Member

valeriangalliat commented Jan 19, 2015

Well, we could say:

  • s,^/\*\*,///, (replace lines that begin with /** by ///).
  • s,^ *\*/,///, (replace lines that begin with potential space plus */ by ///).
  • s,^ *\*,///, (replace lines that begin with potential space plus * by ///).

So:

find . -name '*.s[ac]ss' -exec sed -i '' 's,^/\*\*,///,;s,^ *\*/,///,;s,^ *\*,///,' {} + # BSD/Mac `sed`
find . -name '*.s[ac]ss' -exec sed -i 's,^/\*\*,///,;s,^ *\*/,///,;s,^ *\*,///,' {} + # GNU `sed`

But by all means, this is regex, this is not parser.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

But by all means, this is regex, this is not parser.

Better than nothing.

Member

HugoGiraudel commented Jan 19, 2015

But by all means, this is regex, this is not parser.

Better than nothing.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

BTW the above regexes were not tested please nobody try this without version control.

@HugoGiraudel do you have some SCSS using C-style comments so I can try?

Member

valeriangalliat commented Jan 19, 2015

BTW the above regexes were not tested please nobody try this without version control.

@HugoGiraudel do you have some SCSS using C-style comments so I can try?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

Not that I am aware of.

Member

HugoGiraudel commented Jan 19, 2015

Not that I am aware of.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

This reflects the need for a migration script…

Member

valeriangalliat commented Jan 19, 2015

This reflects the need for a migration script…

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jan 19, 2015

Member

Will add an option to Cdocparser

Member

FWeinb commented Jan 19, 2015

Will add an option to Cdocparser

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

Terrific.

Member

HugoGiraudel commented Jan 19, 2015

Terrific.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

@HugoGiraudel We're both dumb I think. :trollface:

Here we have SCSS using C-style comments. SassDoc/sassdoc.github.io#96

Member

valeriangalliat commented Jan 19, 2015

@HugoGiraudel We're both dumb I think. :trollface:

Here we have SCSS using C-style comments. SassDoc/sassdoc.github.io#96

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

So the script works well, but it fuck up all Markdown bullet lists. I ran a variant where the space before the * is not optional to avoid this.

Do we keep empty comments above and bellow the documentation, where /** and */ used to be?

Member

valeriangalliat commented Jan 19, 2015

So the script works well, but it fuck up all Markdown bullet lists. I ran a variant where the space before the * is not optional to avoid this.

Do we keep empty comments above and bellow the documentation, where /** and */ used to be?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

I removed all references to C-style comments in the docs: SassDoc/develop#1.

@valeriangalliat you need to add your script to the Upgrading page. When it's clear what's going on (deprecating, dropping...), I'll add it to the changelog.

Member

HugoGiraudel commented Jan 19, 2015

I removed all references to C-style comments in the docs: SassDoc/develop#1.

@valeriangalliat you need to add your script to the Upgrading page. When it's clear what's going on (deprecating, dropping...), I'll add it to the changelog.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

I'll add the script when it's clear what we're doing.

Member

valeriangalliat commented Jan 19, 2015

I'll add the script when it's clear what we're doing.

@HugoGiraudel HugoGiraudel removed the Maybe label Jan 19, 2015

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

Final scripts:

Pure sed:

# Opening (can't determine if poster or normal)
s,^/\*\*,///,

# Poster closing
s,^  *\*\*/,////,

# Normal closing
s,^  *\*/,///,

# Comment body
s,^  *\*,///,

GNU sed:

find . -name '*.s[ac]ss' -exec sed -i 's,^/\*\*,///,;s,^  *\*\*/,////,;s,^  *\*/,///,;s,^  *\*,///,' {} +

Mac/BSD sed:

find . -name '*.s[ac]ss' -exec sed -i '' 's,^/\*\*,///,;s,^  *\*\*/,////,;s,^  *\*/,///,;s,^  *\*,///,' {} +

Please make sure to review all your poster comments by hand since there is no way for this script to convert them accurately (only the closing can be converted, but you'll need to add a / to all poster openings).

Member

valeriangalliat commented Jan 19, 2015

Final scripts:

Pure sed:

# Opening (can't determine if poster or normal)
s,^/\*\*,///,

# Poster closing
s,^  *\*\*/,////,

# Normal closing
s,^  *\*/,///,

# Comment body
s,^  *\*,///,

GNU sed:

find . -name '*.s[ac]ss' -exec sed -i 's,^/\*\*,///,;s,^  *\*\*/,////,;s,^  *\*/,///,;s,^  *\*,///,' {} +

Mac/BSD sed:

find . -name '*.s[ac]ss' -exec sed -i '' 's,^/\*\*,///,;s,^  *\*\*/,////,;s,^  *\*/,///,;s,^  *\*,///,' {} +

Please make sure to review all your poster comments by hand since there is no way for this script to convert them accurately (only the closing can be converted, but you'll need to add a / to all poster openings).

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jan 19, 2015

Member

And it's dropped 9f6e4ae

Member

FWeinb commented Jan 19, 2015

And it's dropped 9f6e4ae

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

To do:

  • update Changelog;
  • udpate Upgrading.
Member

HugoGiraudel commented Jan 19, 2015

To do:

  • update Changelog;
  • udpate Upgrading.
@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 19, 2015

Member

I take upgrading.

Member

valeriangalliat commented Jan 19, 2015

I take upgrading.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 19, 2015

Member

I take changelog.

Member

HugoGiraudel commented Jan 19, 2015

I take changelog.

@monte-hayward

This comment has been minimized.

Show comment
Hide comment
@monte-hayward

monte-hayward Apr 21, 2016

It seems to me we are loosing a powerful standard here. In Java, and JavaScript, these comment styles have very specific meanings. Since we have these comment types available to us in Scss, why not leverage them?

/**MyClass - document Myclass here
 * @param,  @return, etc
 **/
var MyClass = {
    more.code.here();
};  // some dev notes

/*  this is 
    a multi-line comment, 
    not intended for the public documentation
*/

// This is single-line. Omit it from docs.
someCodeHere(s);  // e.g. "fixes bug#22"

monte-hayward commented Apr 21, 2016

It seems to me we are loosing a powerful standard here. In Java, and JavaScript, these comment styles have very specific meanings. Since we have these comment types available to us in Scss, why not leverage them?

/**MyClass - document Myclass here
 * @param,  @return, etc
 **/
var MyClass = {
    more.code.here();
};  // some dev notes

/*  this is 
    a multi-line comment, 
    not intended for the public documentation
*/

// This is single-line. Omit it from docs.
someCodeHere(s);  // e.g. "fixes bug#22"
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Apr 21, 2016

Member

We had a few motives for this:

  1. CSS Guidelines and Sass Guidelines both recommend to use inline comments to document Sass code and to reserve C-style comments for CSS.
  2. C-style SassDoc comments in libraries caused issues when included in CodePen or SassMeister because they polluted the output with tons of void comments (sometimes thousands of lines), making these libraries annoying to use on playgrounds.
Member

HugoGiraudel commented Apr 21, 2016

We had a few motives for this:

  1. CSS Guidelines and Sass Guidelines both recommend to use inline comments to document Sass code and to reserve C-style comments for CSS.
  2. C-style SassDoc comments in libraries caused issues when included in CodePen or SassMeister because they polluted the output with tons of void comments (sometimes thousands of lines), making these libraries annoying to use on playgrounds.
@monte-hayward

This comment has been minimized.

Show comment
Hide comment
@monte-hayward

monte-hayward Apr 21, 2016

I trust you have your reasons. 2. sounds like a parse bug in codepen and Sassmeister

monte-hayward commented Apr 21, 2016

I trust you have your reasons. 2. sounds like a parse bug in codepen and Sassmeister

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Apr 21, 2016

@monte-hayward It's not a parse bug. In Sass, C-style comments are used to annotate resulting CSS, while line comments are used to annotate source code.

lolmaus commented Apr 21, 2016

@monte-hayward It's not a parse bug. In Sass, C-style comments are used to annotate resulting CSS, while line comments are used to annotate source code.

@dietergeerts

This comment has been minimized.

Show comment
Hide comment
@dietergeerts

dietergeerts Jun 9, 2018

@lolmaus , then why doesn't the parser has the option to remove such comments before outputting the final css? Then there would be no need to stop using then, as they are more descriptive and readable.
I came here because I wanted to run the parsing part on actual css files, which have these c-style comments in them, which will be removed by our build process.

dietergeerts commented Jun 9, 2018

@lolmaus , then why doesn't the parser has the option to remove such comments before outputting the final css? Then there would be no need to stop using then, as they are more descriptive and readable.
I came here because I wanted to run the parsing part on actual css files, which have these c-style comments in them, which will be removed by our build process.

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Jun 12, 2018

@dietergeerts Why do you ask me? 😬

But if you need to remove all comments, you can do so with a PostCSS filter. Sass and PostCSS are a great pair, autoprefixer FTW!

lolmaus commented Jun 12, 2018

@dietergeerts Why do you ask me? 😬

But if you need to remove all comments, you can do so with a PostCSS filter. Sass and PostCSS are a great pair, autoprefixer FTW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment