@requires Ruby/vendor items #61

Closed
pascalduez opened this Issue Jul 7, 2014 · 25 comments

Comments

Projects
None yet
6 participants
@pascalduez
Member

pascalduez commented Jul 7, 2014

We should find a way to @requires functions declared in Ruby, while extending Sass::Script::Functions.
Right now it logs a message, which is great, but the function don't get printed in the requires.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 7, 2014

Member

Which makes me think, it would be also be neat to be able to @requires third parties dependencies:
@requires compass::sprites::sprite-position
@requires SassyList::reverse
Something in that fashion.

Member

pascalduez commented Jul 7, 2014

Which makes me think, it would be also be neat to be able to @requires third parties dependencies:
@requires compass::sprites::sprite-position
@requires SassyList::reverse
Something in that fashion.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 7, 2014

Member

What about @requires {native} item?

Member

HugoGiraudel commented Jul 7, 2014

What about @requires {native} item?

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 7, 2014

Member

So @requires {native} item for the project own functions.
And maybe @requires {vendor} SassyLists.reverse for third parties ?
Or @requires {vendor} reverse from SassyLists

Member

pascalduez commented Jul 7, 2014

So @requires {native} item for the project own functions.
And maybe @requires {vendor} SassyLists.reverse for third parties ?
Or @requires {vendor} reverse from SassyLists

@HugoGiraudel HugoGiraudel added this to the 1.1 milestone Jul 7, 2014

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 7, 2014

Member

Why not include the link to the documentation of the third party:
@requires {vendor} SassyList.reverse (http://sassylists.com/documentation.html#sl-reverse)

Member

FWeinb commented Jul 7, 2014

Why not include the link to the documentation of the third party:
@requires {vendor} SassyList.reverse (http://sassylists.com/documentation.html#sl-reverse)

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 7, 2014

Member

Why not include the link to the documentation of the third party:

Even better !

Member

pascalduez commented Jul 7, 2014

Why not include the link to the documentation of the third party:

Even better !

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 7, 2014

Member

Why not include the link to the documentation of the third party:

Because the third party may not have a documentation.

Member

HugoGiraudel commented Jul 7, 2014

Why not include the link to the documentation of the third party:

Because the third party may not have a documentation.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 7, 2014

Member

But if it has it would be great. De should at least allow it.

Member

FWeinb commented Jul 7, 2014

But if it has it would be great. De should at least allow it.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
Member

HugoGiraudel commented Jul 7, 2014

Yep.

@HugoGiraudel HugoGiraudel changed the title from @requires ruby declared functions to @requires Ruby/vendor items Jul 8, 2014

@aaronlademann-wf

This comment has been minimized.

Show comment
Hide comment
@aaronlademann-wf

aaronlademann-wf Jul 8, 2014

I think the syntax for the vendor stuff should be linked to how it is imported... This may allow for the {context} specifier to remain consistent with non-vendor/lib requirements, specifying function|mixin|variable.

For instance (using dot-notation)...

@import "SassyLists";
@import "sassy-maps";

/**
 * @requires {function} SassyLists.function-name
 * @requires {mixin} sassy-maps.mixin-name
 */

Or (using slash/dir notation)...

@import "SassyLists";
@import "sassy-maps";

/**
 * @requires {function} SassyLists/function-name
 * @requires {mixin} sassy-maps/mixin-name
 */

I think the syntax for the vendor stuff should be linked to how it is imported... This may allow for the {context} specifier to remain consistent with non-vendor/lib requirements, specifying function|mixin|variable.

For instance (using dot-notation)...

@import "SassyLists";
@import "sassy-maps";

/**
 * @requires {function} SassyLists.function-name
 * @requires {mixin} sassy-maps.mixin-name
 */

Or (using slash/dir notation)...

@import "SassyLists";
@import "sassy-maps";

/**
 * @requires {function} SassyLists/function-name
 * @requires {mixin} sassy-maps/mixin-name
 */
@aaronlademann-wf

This comment has been minimized.

Show comment
Hide comment
@aaronlademann-wf

aaronlademann-wf Jul 8, 2014

Also - I don't think this should be limited to "Ruby" libraries... but rather anything that is being required and imported (vendor Sass libraries, for instance).

Also - I don't think this should be limited to "Ruby" libraries... but rather anything that is being required and imported (vendor Sass libraries, for instance).

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 9, 2014

Member

@valeriangalliat, I'd like your input on this if you don't mind. :)

Member

HugoGiraudel commented Jul 9, 2014

@valeriangalliat, I'd like your input on this if you don't mind. :)

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jul 9, 2014

Member

I like Aaron's proposition.

I'm not sure about a dot, double colon or slash notation. SassDoc isn't strict about syntax, so we can just allow a list of delimiters (::, ., /), and split the @requires value with them.

It's also a good idea to be able to provide a link to the documentation, but I'm not sure if it should be surrounded by parens... is there something standard (at least in SassDoc) for this?

Maybe allow a description too (optional, like the URL), so we can tell why the dependency is required.

Member

valeriangalliat commented Jul 9, 2014

I like Aaron's proposition.

I'm not sure about a dot, double colon or slash notation. SassDoc isn't strict about syntax, so we can just allow a list of delimiters (::, ., /), and split the @requires value with them.

It's also a good idea to be able to provide a link to the documentation, but I'm not sure if it should be surrounded by parens... is there something standard (at least in SassDoc) for this?

Maybe allow a description too (optional, like the URL), so we can tell why the dependency is required.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 9, 2014

Member

The problem I can see with Aaron's proposal is that SassyLists.reverse won't be treated because it doesn't exist in the index. We have to find a way to describe a dependency that is completely outside of the documented code.

It's also a good idea to be able to provide a link to the documentation

I don't want that. We have @link to define a link to somewhere on the internet, let's not duplicate things.

Regarding description, that might be an idea. This is a totally different issue though, I will open it shortly so we can discuss it.

Member

HugoGiraudel commented Jul 9, 2014

The problem I can see with Aaron's proposal is that SassyLists.reverse won't be treated because it doesn't exist in the index. We have to find a way to describe a dependency that is completely outside of the documented code.

It's also a good idea to be able to provide a link to the documentation

I don't want that. We have @link to define a link to somewhere on the internet, let's not duplicate things.

Regarding description, that might be an idea. This is a totally different issue though, I will open it shortly so we can discuss it.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jul 9, 2014

Member

Well we can assume if the dependency don't exists in the index, it's an external dependency and we can just display it as-is (without an anchor).

Or maybe something like:

/**
 * @require-external {function} SassyLists.reverse Maybe a description <http://sassylists.com/documentation.html#sl-reverse>
 */

... but probably with another name, and I'm not sure with the format either.

Member

valeriangalliat commented Jul 9, 2014

Well we can assume if the dependency don't exists in the index, it's an external dependency and we can just display it as-is (without an anchor).

Or maybe something like:

/**
 * @require-external {function} SassyLists.reverse Maybe a description <http://sassylists.com/documentation.html#sl-reverse>
 */

... but probably with another name, and I'm not sure with the format either.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 9, 2014

Member

Well we can assume if the dependency don't exists in the index, it's an external dependency and we can just display it as-is (without an anchor).

Currently, we are assuming that an incorrect dependency is a mistake on the dev's side. Indeed, we could stop caring and just drop it as is (without any link beneath it).

Member

HugoGiraudel commented Jul 9, 2014

Well we can assume if the dependency don't exists in the index, it's an external dependency and we can just display it as-is (without an anchor).

Currently, we are assuming that an incorrect dependency is a mistake on the dev's side. Indeed, we could stop caring and just drop it as is (without any link beneath it).

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jul 9, 2014

Member

Yep, that's why a specific annotation can be great; we can still prevent dev's mistakes for @requires.

Or if we use @requires for external dependencies, we need a way to figure if it's an external dependency or not.

Well... I don't think you can have a variable, mixin or function containing ::, . or / in its name, so if the dependency name contains one if these delimiters we can assume it's external (so no error, and display as-is). Otherwise we can still warn the dev.

Member

valeriangalliat commented Jul 9, 2014

Yep, that's why a specific annotation can be great; we can still prevent dev's mistakes for @requires.

Or if we use @requires for external dependencies, we need a way to figure if it's an external dependency or not.

Well... I don't think you can have a variable, mixin or function containing ::, . or / in its name, so if the dependency name contains one if these delimiters we can assume it's external (so no error, and display as-is). Otherwise we can still warn the dev.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 9, 2014

Member

Well... I don't think you can have a variable, mixin or function containing ::, . or / in its name, so if the dependency name contains one if these delimiters we can assume it's external (so no error, and display as-is). Otherwise we can still warn the dev.

Seems legit.

Member

HugoGiraudel commented Jul 9, 2014

Well... I don't think you can have a variable, mixin or function containing ::, . or / in its name, so if the dependency name contains one if these delimiters we can assume it's external (so no error, and display as-is). Otherwise we can still warn the dev.

Seems legit.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 10, 2014

Member

@pascalduez and @FWeinb, could I have any opinion on this one please?

Member

HugoGiraudel commented Jul 10, 2014

@pascalduez and @FWeinb, could I have any opinion on this one please?

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 10, 2014

Member

Let's try to resume:

1
Allowing ::, ., / for dependency name is nice +1

2
Allowing a link or reference inside some delimiters <http://...> is nice to have but not critical for me

3
Question is do we use @requires or create a new tag ?
Cause we should still be able to warn if a project own require does not exist.

Member

pascalduez commented Jul 10, 2014

Let's try to resume:

1
Allowing ::, ., / for dependency name is nice +1

2
Allowing a link or reference inside some delimiters <http://...> is nice to have but not critical for me

3
Question is do we use @requires or create a new tag ?
Cause we should still be able to warn if a project own require does not exist.

@alademann

This comment has been minimized.

Show comment
Hide comment
@alademann

alademann Jul 10, 2014

Contributor

Agree that 2 is not critical. On 3, I think using the existing @requires tag makes sense, and I think you could determine if a "missing" requirement is an exception of the name of the requirement does not contain ::, . or /.

Contributor

alademann commented Jul 10, 2014

Agree that 2 is not critical. On 3, I think using the existing @requires tag makes sense, and I think you could determine if a "missing" requirement is an exception of the name of the requirement does not contain ::, . or /.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 10, 2014

Member

Let's recap.

Thanks to #78, @requires now support a description. Let's not forget that. Here is what we could have:

/**
 * @requires {type} name - description <url>
 */

Where:

  • the hyphen is optional, as stated in #81
  • the name can contain /, :: or .
  • the url is the href for the link

Now, in case there is a URL on a non-vendor item, it would override the default anchor.

Everybody okay?

Member

HugoGiraudel commented Jul 10, 2014

Let's recap.

Thanks to #78, @requires now support a description. Let's not forget that. Here is what we could have:

/**
 * @requires {type} name - description <url>
 */

Where:

  • the hyphen is optional, as stated in #81
  • the name can contain /, :: or .
  • the url is the href for the link

Now, in case there is a URL on a non-vendor item, it would override the default anchor.

Everybody okay?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jul 10, 2014

Member

Yup, I like it.

Member

valeriangalliat commented Jul 10, 2014

Yup, I like it.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 10, 2014

Member

Perfect !

Member

pascalduez commented Jul 10, 2014

Perfect !

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 18, 2014

Member

Who's taking this one guys?

+@valeriangalliat @pascalduez @FWeinb

Member

HugoGiraudel commented Jul 18, 2014

Who's taking this one guys?

+@valeriangalliat @pascalduez @FWeinb

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 18, 2014

Member

I take it.

Member

FWeinb commented Jul 18, 2014

I take it.

@FWeinb FWeinb self-assigned this Jul 18, 2014

@FWeinb FWeinb closed this in 3c1bef0 Jul 18, 2014

FWeinb added a commit that referenced this issue Jul 18, 2014

Merge pull request #105 from SassDoc/requires-external
Add support for external requires Fix #61

@valeriangalliat valeriangalliat referenced this issue Jul 20, 2014

Closed

@group #29

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