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

Convert the module system proposal to stable specifications #2777

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@nex3
Copy link
Contributor

nex3 commented Nov 21, 2019

No description provided.

@nex3 nex3 requested review from Awjin and jathak Nov 21, 2019
Copy link
Contributor

Awjin left a comment

Overall LGTM! Just a few minor things.

spec/spec.md Outdated Show resolved Hide resolved
spec/spec.md Show resolved Hide resolved
spec/modules.md Outdated Show resolved Hide resolved
spec/modules.md Outdated Show resolved Hide resolved
### Built-In Module

A *built-in module* is a module defined by the Sass specification or by outside
of the Sass compilation in some implementation-specific way. Modules defined by

This comment has been minimized.

Copy link
@Awjin

Awjin Nov 21, 2019

Contributor

Wording here is a bit awkward

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

Tweaked

spec/built_in_modules/meta.md Show resolved Hide resolved
spec/built_in_modules/meta.md Show resolved Hide resolved
spec/at-rules/use.md Outdated Show resolved Hide resolved
spec/at-rules/import.md Show resolved Hide resolved
spec/at-rules/extend.md Outdated Show resolved Hide resolved
@jathak
jathak approved these changes Nov 26, 2019
Copy link
Member

jathak left a comment

LGTM, except for here, where I was confused by what the section is supposed to mean

spec/spec.md Show resolved Hide resolved
spec/spec.md Outdated Show resolved Hide resolved
spec/syntax.md Show resolved Hide resolved
[scope]: variables.md#scope
* If `scope` is not null, return `scope`'s value of type `type` named `name`.

This comment has been minimized.

Copy link
@jathak

jathak Nov 26, 2019

Member

What happens if scope is null here?

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

We fall through to the next line. The idea is that it works like a programming language:

if (name is a plain identifier or variable) {
  scope = the scope of the innermost block etc etc
  if (scope != null) return scope;
}
is no such member, throw an error.
* If `type` is not "variable" and the current source file contains a top-level
definition of a member of type `type` named `name`:

This comment has been minimized.

Copy link
@jathak

jathak Nov 26, 2019

Member

I'm not really following what this line is saying, especially in relation to the note that follows. Is there an example that illustrates what this section is for?

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

Added an explanation

@@ -0,0 +1,314 @@
# Color Module

This comment has been minimized.

Copy link
@jathak

jathak Nov 26, 2019

Member

Would it make sense to add TODOs or some other indicator for each function (here and in the other modules) whose semantics aren't formally specified yet?

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

I think doing that in general would produce more noise than it's worth, since most things in the spec currently are unspecified. Maybe once it gets more fleshed-out?

Copy link
Contributor Author

nex3 left a comment

Thanks for the review!

spec/at-rules/use.md Outdated Show resolved Hide resolved
@@ -0,0 +1,314 @@
# Color Module

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

I think doing that in general would produce more noise than it's worth, since most things in the spec currently are unspecified. Maybe once it gets more fleshed-out?

spec/modules.md Outdated Show resolved Hide resolved
### Built-In Module

A *built-in module* is a module defined by the Sass specification or by outside
of the Sass compilation in some implementation-specific way. Modules defined by

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

Tweaked

[scope]: variables.md#scope
* If `scope` is not null, return `scope`'s value of type `type` named `name`.

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

We fall through to the next line. The idea is that it works like a programming language:

if (name is a plain identifier or variable) {
  scope = the scope of the innermost block etc etc
  if (scope != null) return scope;
}
is no such member, throw an error.
* If `type` is not "variable" and the current source file contains a top-level
definition of a member of type `type` named `name`:

This comment has been minimized.

Copy link
@nex3

nex3 Nov 27, 2019

Author Contributor

Added an explanation

spec/spec.md Outdated Show resolved Hide resolved
spec/spec.md Show resolved Hide resolved
spec/syntax.md Show resolved Hide resolved
spec/syntax.md Show resolved Hide resolved
@nex3 nex3 merged commit 29f232a into master Nov 27, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@nex3 nex3 deleted the module-system-spec branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.