Bug with parenthesis inside default values #303

Closed
ben-eb opened this Issue Jan 1, 2015 · 22 comments

Comments

Projects
None yet
5 participants
@ben-eb

ben-eb commented Jan 1, 2015

Given this example SCSS:

/**
 * Main colour palette.
 *
 * @prop {Color} main-background   (rgb(61, 75, 92))  - Deep, blueish gray
 *
 * @see {function} colour
 *
 * @type Map
 */
$colours: (
    "main-background": rgb(61, 75, 92)
);

I should expect that it outputs the colour correctly. However, I get an error:

[16:42:07] TypeError in plugin 'gulp-sassdoc'
Message:
    Cannot use 'in' operator to search for 'stack' in unknown color: rgb(61, 75, 92

Is it possible to add support for RGB(a) and HSL(a) values when documenting colours? 😄

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 1, 2015

Member

I can't get the error with SassDoc CLI, but for sure there's a bug with parentheses in default values.

While (for me) the documentation is generated without errors, the default value is parsed as rgb(61, 75, 92 while the description is ) - Deep, blueish gray. I think we're reaching the limit of regex for decent parsing (and JavaScript don't support recursive regex).

Member

valeriangalliat commented Jan 1, 2015

I can't get the error with SassDoc CLI, but for sure there's a bug with parentheses in default values.

While (for me) the documentation is generated without errors, the default value is parsed as rgb(61, 75, 92 while the description is ) - Deep, blueish gray. I think we're reaching the limit of regex for decent parsing (and JavaScript don't support recursive regex).

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 1, 2015

Member

While I think of it, it may not be the cleanest solution, but we can simply support square brackets to define the default value, so there's no ambiguity with nested parentheses (we can just match \[([^\]]*)\]).

Whatever fix we use, it should be ported to property, require and parameter.

Member

valeriangalliat commented Jan 1, 2015

While I think of it, it may not be the cleanest solution, but we can simply support square brackets to define the default value, so there's no ambiguity with nested parentheses (we can just match \[([^\]]*)\]).

Whatever fix we use, it should be ported to property, require and parameter.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jan 1, 2015

Member

👍 I like wrapping defaults in [].

Member

FWeinb commented Jan 1, 2015

👍 I like wrapping defaults in [].

@valeriangalliat valeriangalliat added this to the 2.0 milestone Jan 1, 2015

@valeriangalliat valeriangalliat self-assigned this Jan 1, 2015

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 2, 2015

Member

Let me consider this, but that might be a decent solution.

Member

HugoGiraudel commented Jan 2, 2015

Let me consider this, but that might be a decent solution.

@HugoGiraudel HugoGiraudel changed the title from Support default values for colours other than hex to Bug with parenthesis inside default values Jan 2, 2015

@HugoGiraudel HugoGiraudel added Maybe and removed Docs Improvement labels Jan 2, 2015

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 2, 2015

Member

This is a major break. All comments will have to be updated to fix this. This kinda sucks imo...

Member

HugoGiraudel commented Jan 2, 2015

This is a major break. All comments will have to be updated to fix this. This kinda sucks imo...

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 2, 2015

Member

We can keep both, at least for 2.0. Though only [] will work properly for defaut values containing parentheses.

Member

valeriangalliat commented Jan 2, 2015

We can keep both, at least for 2.0. Though only [] will work properly for defaut values containing parentheses.

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Jan 2, 2015

I would favour keeping the current style of documentation with parentheses to avoid breakage, and instead fix the parsing logic. It might be enough to use some kind of counter for parens and loop over the characters of a line. You could probably still use regexes for testing that the line resembled a @prop or @since etc, but having a parser that could produce this might be handy:

/**
 * @prop {Color} main-background   (rgb(61, 75, 92))  - Deep, blueish gray
 */
{
    annotation: "prop",
    declaration: {
        type: ["Color"],
        variable: "main-background",
        defaultValue: "rgb(61, 75, 92)",
        description: "Deep, blueish gray"
    }
}

ben-eb commented Jan 2, 2015

I would favour keeping the current style of documentation with parentheses to avoid breakage, and instead fix the parsing logic. It might be enough to use some kind of counter for parens and loop over the characters of a line. You could probably still use regexes for testing that the line resembled a @prop or @since etc, but having a parser that could produce this might be handy:

/**
 * @prop {Color} main-background   (rgb(61, 75, 92))  - Deep, blueish gray
 */
{
    annotation: "prop",
    declaration: {
        type: ["Color"],
        variable: "main-background",
        defaultValue: "rgb(61, 75, 92)",
        description: "Deep, blueish gray"
    }
}
@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jan 2, 2015

Member

Our plugin architecture would infact allow us to do that. We would need to refactor all annotations that are using a default value. We might need to do that. Have a look here

Member

FWeinb commented Jan 2, 2015

Our plugin architecture would infact allow us to do that. We would need to refactor all annotations that are using a default value. We might need to do that. Have a look here

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 2, 2015

Member

to avoid breakage

As I said before, we can still support the (flawed) parentheses for default value, to maintain BC, but also add support for square brackets (with the same semantic), though not having any problem with nested parentheses.

It definitely doesn't solve the problem with parentheses parsing, but at least it provides an easy way to circumvent it. Quick and easy to implement, but the downside is you have to replace parentheses with square brackets when needed.

BTW square brackets for default values would be consistent with Unix commands synopsis, and not unlike JSDoc, whose syntax is slightly different but still uses square brackets for optional arguments / default values.

fix the parsing logic

Regardless of the consistency with Unix commands and JSDoc, that's indeed the obvious way to fix the problem in good standing. However, as @FWeinb pointed out, it would require us to refactor all the annotations (and I wouldn't limit it to "using a default value", since it would look somehow odd to have a "real" annotations parser for some annotations, and "dumb" regex parsing for others).

While we might need a parser for annotations someday, I would really consider moving to square brackets for default values for the above reasons (while maintaining BC with parentheses for non-problematic default values).

And I'm afraid if we want to parse all default values, we're again needing an actual SCSS parser.

Member

valeriangalliat commented Jan 2, 2015

to avoid breakage

As I said before, we can still support the (flawed) parentheses for default value, to maintain BC, but also add support for square brackets (with the same semantic), though not having any problem with nested parentheses.

It definitely doesn't solve the problem with parentheses parsing, but at least it provides an easy way to circumvent it. Quick and easy to implement, but the downside is you have to replace parentheses with square brackets when needed.

BTW square brackets for default values would be consistent with Unix commands synopsis, and not unlike JSDoc, whose syntax is slightly different but still uses square brackets for optional arguments / default values.

fix the parsing logic

Regardless of the consistency with Unix commands and JSDoc, that's indeed the obvious way to fix the problem in good standing. However, as @FWeinb pointed out, it would require us to refactor all the annotations (and I wouldn't limit it to "using a default value", since it would look somehow odd to have a "real" annotations parser for some annotations, and "dumb" regex parsing for others).

While we might need a parser for annotations someday, I would really consider moving to square brackets for default values for the above reasons (while maintaining BC with parentheses for non-problematic default values).

And I'm afraid if we want to parse all default values, we're again needing an actual SCSS parser.

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Jan 2, 2015

I don't really have the time at the moment to come up with some parser that can solve these problems, but it is an area that I have a lot of interest in. So, perhaps one day I might go create one, but yes indeed it seems that just replacing the parentheses with square brackets is a good solution for now. 👍

ben-eb commented Jan 2, 2015

I don't really have the time at the moment to come up with some parser that can solve these problems, but it is an area that I have a lot of interest in. So, perhaps one day I might go create one, but yes indeed it seems that just replacing the parentheses with square brackets is a good solution for now. 👍

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 2, 2015

Member

If we are going to move brackets, there is no double support. We get rid off parenthesis.

Member

HugoGiraudel commented Jan 2, 2015

If we are going to move brackets, there is no double support. We get rid off parenthesis.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 2, 2015

Member

I don't really have the time at the moment to come up with some parser, but it is an area that I have a lot of interest in.

That makes two of us. 😄

If we are going to move brackets, there is no double support. We get rid off parenthesis.

Okay.

So, do we want:

@prop {Color} main-background [rgb(61, 75, 92)]  - Deep, blueish gray

… or:

@prop {Color} [main-background=rgb(61, 75, 92)]  - Deep, blueish gray

… like JSDoc?

Member

valeriangalliat commented Jan 2, 2015

I don't really have the time at the moment to come up with some parser, but it is an area that I have a lot of interest in.

That makes two of us. 😄

If we are going to move brackets, there is no double support. We get rid off parenthesis.

Okay.

So, do we want:

@prop {Color} main-background [rgb(61, 75, 92)]  - Deep, blueish gray

… or:

@prop {Color} [main-background=rgb(61, 75, 92)]  - Deep, blueish gray

… like JSDoc?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 2, 2015

Member

The JSDoc syntax is ugly as fuck.

Member

HugoGiraudel commented Jan 2, 2015

The JSDoc syntax is ugly as fuck.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 2, 2015

Member

You've got a point.

Member

valeriangalliat commented Jan 2, 2015

You've got a point.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 2, 2015

Member

BTW here's a migration script if we replace parentheses with square brackets.

GNU sed
find . -type f -name '*.s[ac]ss' -exec sed -ri '/@param|@prop|@require/y/()/[]/' {} +
BSD/Mac sed
find . -type f -name '*.s[ac]ss' -exec sed -Ei '' '/@param|@prop|@require/y/\(\)/\[\]/' {} +
Member

valeriangalliat commented Jan 2, 2015

BTW here's a migration script if we replace parentheses with square brackets.

GNU sed
find . -type f -name '*.s[ac]ss' -exec sed -ri '/@param|@prop|@require/y/()/[]/' {} +
BSD/Mac sed
find . -type f -name '*.s[ac]ss' -exec sed -Ei '' '/@param|@prop|@require/y/\(\)/\[\]/' {} +
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 2, 2015

Member

I like that.

Member

HugoGiraudel commented Jan 2, 2015

I like that.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 2, 2015

Member

Okay to move to []. @SassDoc/owners, all okay?

Member

HugoGiraudel commented Jan 2, 2015

Okay to move to []. @SassDoc/owners, all okay?

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jan 2, 2015

Member

👍

Member

FWeinb commented Jan 2, 2015

👍

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jan 2, 2015

Member

Fine.

Member

pascalduez commented Jan 2, 2015

Fine.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 2, 2015

Member

I'll implement this in the coming days.

  • Replace parentheses by square brackets in @param, @prop and @require annotations.
  • Update the documentation.
Member

valeriangalliat commented Jan 2, 2015

I'll implement this in the coming days.

  • Replace parentheses by square brackets in @param, @prop and @require annotations.
  • Update the documentation.
@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jan 5, 2015

Member

Merged.

Anyone have an idea where to put the migration script?

Member

valeriangalliat commented Jan 5, 2015

Merged.

Anyone have an idea where to put the migration script?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 5, 2015

Member

Open an issue on SassDoc's site.

Member

HugoGiraudel commented Jan 5, 2015

Open an issue on SassDoc's site.

@valeriangalliat valeriangalliat referenced this issue in SassDoc/sassdoc.github.io Jan 5, 2015

Closed

Where to put migration scripts for 2.0? #80

0 of 1 task complete

@valeriangalliat valeriangalliat referenced this issue in lunelson/sass-maps-plus Jan 14, 2015

Merged

Publishing sassdoc docs #12

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