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

Auto-fill @requires #232

Closed
HugoGiraudel opened this Issue Oct 1, 2014 · 20 comments

Comments

Projects
None yet
4 participants
@HugoGiraudel
Member

HugoGiraudel commented Oct 1, 2014

What if we had an extension to auto-fill @requires annotation? That would need to be quite clever though, because we don't want to add local variables and Sass native functions as required.

Any thought?

@HugoGiraudel HugoGiraudel added the Feature label Oct 1, 2014

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Oct 1, 2014

Member

That would be quite handy.
Having to maintain the list of dependencies can turn a bit tiresome at times.

What do you mean by extension ? A new annotation that would override the core one ?

Maintaining a list of Sass core functions looks a bit uneficient, maybe we could find a list to parse.

It's gonna be tricky to differentiate local from global scope vars.

Member

pascalduez commented Oct 1, 2014

That would be quite handy.
Having to maintain the list of dependencies can turn a bit tiresome at times.

What do you mean by extension ? A new annotation that would override the core one ?

Maintaining a list of Sass core functions looks a bit uneficient, maybe we could find a list to parse.

It's gonna be tricky to differentiate local from global scope vars.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 1, 2014

Member

What do you mean by extension ? A new annotation that would override the core one ?

No. I mean an extension in some kind of way. Everybody won't want this I think.

Member

HugoGiraudel commented Oct 1, 2014

What do you mean by extension ? A new annotation that would override the core one ?

No. I mean an extension in some kind of way. Everybody won't want this I think.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 1, 2014

Member

That could be done like the @content annotation searching though the code. But if we do more things like this (searching the code) we should update the scss-comment-parser to include a way to invoke an annotation if it isn't found. To keep performance up.

Member

FWeinb commented Oct 1, 2014

That could be done like the @content annotation searching though the code. But if we do more things like this (searching the code) we should update the scss-comment-parser to include a way to invoke an annotation if it isn't found. To keep performance up.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 4, 2014

Member

@SassDoc/owners Do you feel like this could be done in 1.x? For instance 1.10?

Member

HugoGiraudel commented Oct 4, 2014

@SassDoc/owners Do you feel like this could be done in 1.x? For instance 1.10?

@FWeinb FWeinb referenced this issue Oct 5, 2014

Closed

Auto-fill @throws #238

@HugoGiraudel HugoGiraudel modified the milestone: 1.10 Oct 5, 2014

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 5, 2014

Member

Just doing a chat dump here from Hugo:

Mixins

Mixins are easy because Sass doesn't provide built-in mixins. So if you meet @include, you know it's a required mixin.

Placeholders

Placeholders are easy because Sass doesn't provide built-in placeholders. So if you meet @extend %, you know it's a required placeholder.

Functions:

For functions, we should have a list of built-in functions. When we meet .+(, if the string is not in the array of built in functions, it's a required one.

The only real problem is to detect used variables.

Any idea on how to determine if a variable is used @SassDoc/owners?

Member

FWeinb commented Oct 5, 2014

Just doing a chat dump here from Hugo:

Mixins

Mixins are easy because Sass doesn't provide built-in mixins. So if you meet @include, you know it's a required mixin.

Placeholders

Placeholders are easy because Sass doesn't provide built-in placeholders. So if you meet @extend %, you know it's a required placeholder.

Functions:

For functions, we should have a list of built-in functions. When we meet .+(, if the string is not in the array of built in functions, it's a required one.

The only real problem is to detect used variables.

Any idea on how to determine if a variable is used @SassDoc/owners?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 5, 2014

Member
  1. Start at beginning of scope.
  2. Read until end of scope.
  3. Every time you find a variable assignment (e.g. $a: ...;) that does not use !global, store it in an array.
  4. Get back at beginning of scope.
  5. Read until end of scope.
  6. Every time you find a variable use (e.g. $a), test if it belongs to the previously built array. If it does not, it's a global hence required variable.

You can probably improve performance by tweaking the algorithm to include check at run time.

Member

HugoGiraudel commented Oct 5, 2014

  1. Start at beginning of scope.
  2. Read until end of scope.
  3. Every time you find a variable assignment (e.g. $a: ...;) that does not use !global, store it in an array.
  4. Get back at beginning of scope.
  5. Read until end of scope.
  6. Every time you find a variable use (e.g. $a), test if it belongs to the previously built array. If it does not, it's a global hence required variable.

You can probably improve performance by tweaking the algorithm to include check at run time.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 5, 2014

Member

Currently it only makes sense to have a usedBy <-> required association between documented variables. So if we assume that we could just dump all found variables as the default value into the requires array of an item and clean them out in the resolve function. If we don't find a variable (thus it is not documented) we just ignore it.

Member

FWeinb commented Oct 5, 2014

Currently it only makes sense to have a usedBy <-> required association between documented variables. So if we assume that we could just dump all found variables as the default value into the requires array of an item and clean them out in the resolve function. If we don't find a variable (thus it is not documented) we just ignore it.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 5, 2014

Member

Makes sense I suppose.

Member

HugoGiraudel commented Oct 5, 2014

Makes sense I suppose.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 5, 2014

Member

I have an initial draft here.

It is working quite well so far, but we need to think about how we want to handle auto-filled annotations. Currently a annotation can only be auto-filled if it wasn't manually used by the author, in many cases that is a good behaviour, but in some cases (like for requires) I would be great if an author could manually extend the annotations. (like adding an external requires?).

If we want to support this I think adding a extend key to the annotations API would work quite nice here. So we could have a default value if the user doesn't used the annotation. And extend it if the user has some values.

@SassDoc/owners
Should we support that?

Member

FWeinb commented Oct 5, 2014

I have an initial draft here.

It is working quite well so far, but we need to think about how we want to handle auto-filled annotations. Currently a annotation can only be auto-filled if it wasn't manually used by the author, in many cases that is a good behaviour, but in some cases (like for requires) I would be great if an author could manually extend the annotations. (like adding an external requires?).

If we want to support this I think adding a extend key to the annotations API would work quite nice here. So we could have a default value if the user doesn't used the annotation. And extend it if the user has some values.

@SassDoc/owners
Should we support that?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 5, 2014

Member

While it makes perfect sense, I'm afraid about overall complexity.

That being said, extend could be used file level annotations as well, where items would extend annotations from poster rather than overriding those.

Member

HugoGiraudel commented Oct 5, 2014

While it makes perfect sense, I'm afraid about overall complexity.

That being said, extend could be used file level annotations as well, where items would extend annotations from poster rather than overriding those.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 6, 2014

Member

Good point @HugoGiraudel, so should I implement it or not @SassDoc/owners?

Member

FWeinb commented Oct 6, 2014

Good point @HugoGiraudel, so should I implement it or not @SassDoc/owners?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Oct 6, 2014

Member

That's definitely a nice feature, helpful. Let's do it if it stays below a reasonable amount of complexity ?
Maybe add an option to opt out ? in .sassdocrc ?

Member

pascalduez commented Oct 6, 2014

That's definitely a nice feature, helpful. Let's do it if it stays below a reasonable amount of complexity ?
Maybe add an option to opt out ? in .sassdocrc ?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 7, 2014

Member

@valeriangalliat, I want your opinion on this please. :)

Member

HugoGiraudel commented Oct 7, 2014

@valeriangalliat, I want your opinion on this please. :)

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Oct 7, 2014

Member

I don't understand the extend problem. We can analyze the code syntax in resolve and push everything we find in item.requires array, right?

Though I'm not sure beginning to do our own lexical analysis on Sass code is a good idea. To me, if we want something bulletproff, it basically means reimplementing the standard Sass lexer (or even parser) in Node.js (or better, bind libsass parser to Node.js to browse a proper and reliable syntax tree).

While this is an interesting thing, I think it involves too much complexity.

Member

valeriangalliat commented Oct 7, 2014

I don't understand the extend problem. We can analyze the code syntax in resolve and push everything we find in item.requires array, right?

Though I'm not sure beginning to do our own lexical analysis on Sass code is a good idea. To me, if we want something bulletproff, it basically means reimplementing the standard Sass lexer (or even parser) in Node.js (or better, bind libsass parser to Node.js to browse a proper and reliable syntax tree).

While this is an interesting thing, I think it involves too much complexity.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 7, 2014

Member

@valeriangalliat I see your objections. The only problem I can see is that I don't remove strings before searching for mixin/functions/placeholders currently. You might want to have a look here

Member

FWeinb commented Oct 7, 2014

@valeriangalliat I see your objections. The only problem I can see is that I don't remove strings before searching for mixin/functions/placeholders currently. You might want to have a look here

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Oct 7, 2014

Member

@FWeinb Strings, and comments, I think.

Though the probability to have a comment or a string with something looking like a variable/placeholder in it, in a block where this variable/placeholder is not used seems really low to me, so I guess that's acceptable. :)

Member

valeriangalliat commented Oct 7, 2014

@FWeinb Strings, and comments, I think.

Though the probability to have a comment or a string with something looking like a variable/placeholder in it, in a block where this variable/placeholder is not used seems really low to me, so I guess that's acceptable. :)

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 7, 2014

Member

We would just need a way to disable these autofills on a per comment basis.

Maybe we use a annotation called autofill that can hold allowed annotations that will be autofilled.

Member

FWeinb commented Oct 7, 2014

We would just need a way to disable these autofills on a per comment basis.

Maybe we use a annotation called autofill that can hold allowed annotations that will be autofilled.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 8, 2014

Member

How would it work exactly @FWeinb?

Member

HugoGiraudel commented Oct 8, 2014

How would it work exactly @FWeinb?

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Oct 8, 2014

Member

Made a PR for review

Member

FWeinb commented Oct 8, 2014

Made a PR for review

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