Restructure directories to conform to style guide

[Awjin]

The directory structure has grown organically over time, and as such does not completely adhere to the recommendations set in the style guide.

For example, some directories have generic names that don't describe their function (basic/, misc/, etc). Some are redundant and could be combined with other directories (colors/, mixin/, operations/, etc). Some files are at root and could be reorganized into sub-directories.

We should decide on an ideal directory structure, and move specs into that structure.

[saper]

#1484 has a discussion whether we need regression tests (organized by issue, not by the feature they test) in the repository as well.

[Awjin]

Thanks for the context. My personal opinion is that regression tests should be added to the feature, so that over time the feature specs become more robust.

Regardless, the bulk of reorganization can be done without touching the regression tests, so I think this work should proceed in parallel.

[saper]

Thank you. Yes, I fully agree that there is enough work to be done in the spec (feature) tests alone.

[Awjin]

Here's a first pass at a new structure for spec/, excluding libsass directories.

Current Top-Level Directories

basic/
colors/
core_functions/
css/
directives/
errors/
extend-tests/
misc/
mixin/
nesting/
operations/
parser/
sass/
sass_4_0/
scope/
scss/
scss-tests/
selectors/
test.scss/
values/

Don't Need Reorganization

core_functions/
css/
directives/
operations/
values/

Need Reorganization

Move the following redundant directories into existing directories

colors/ —> values/colors/
extend-tests/ -> directives/extend/
media-import.hrx -> directives/import/
mixin/ -> directives/mixin/

Move the following generic specs into the feature that they actually test

basic/
errors/
misc/
sass/
sass_4_0/
scss/
scss-tests/

New top-level directories

• variables/

scope/ -> variables/scope/

• parser/selectors/ becomes top-level selectors/

Rename parser/ to syntax/, then move files into it

nesting/ -> syntax/nesting/
sass/, scss/ -> Instead of grouping syntax weirdness into sass/ and scss/,
                have a spec for each feature that causes the syntax to behave
                weirdly. These specs should each contain sass and scss inputs.

Proposed Directory Structure

core_functions/
css/
directives/
operations/
selectors/
syntax/
values/
variables/

[Awjin]

I'm struggling most with the somewhat generic syntax folder—could this be made more explicit, or split up even further?

[nex3]

Something to consider is how we track tests that we think conform well to the style guide versus those that do not. Right now, we can rely on the top-level directories as a signal here—the directories under "doesn't need reorg" are all new directories that generally follow the style guide, and the older directories generally do not. But if we want to move existing specs into a better directory structure before we style-guide-ify them, we'll need another signal to track things like "how many specs are following the style guide?" and "where are specs that aren't following the style guide and thus need fixing?"

One option would be to design an ideal directory structure in this issue, but continue to only actually put files into it as they go through the style-guide-ification process.

Move redundant directories into existing directories

colors/ —> values/colors/
extend-tests/ -> directives/extend/
media-import.hrx -> directives/import/
mixin/ -> directives/mixin/

Mostly SGTM, although I think the media import tests should live in css/ since that's only a feature of plain CSS imports.

Move generic specs into the feature that they actually test

basic/
errors/
misc/
sass/
sass_4_0/
scss/
scss-tests/

SGTM

New top-level directories

• variables/

variables.hrx -> variables/
scope/ -> variables/scope/

• parser/selectors/ becomes top-level selectors/

Does it make sense to make scope/ a subdirectory of variables/? A lot of scoping logic (other than reassignment) applies to mixins and functions as well.

Should selectors/ be style_rules/, to match the general principle of categorizing specs by the statement-level construct that they refer to?

Rename parser/ to syntax/, then move files into it

nesting/ -> syntax/nesting/
sass/, scss/ -> Instead of grouping syntax weirdness into sass/ and scss/,
                have a spec for each feature that causes the syntax to behave
                weirdly, then for each feature spec have both sass and scss
                inputs.

What are you envisioning would fall under "syntax" as opposed to any other category? It seems like kind of a catch-all.

What about specs that only make sense in one syntax or the other? (Vagaries of indentation for the indented syntax, semicolons and brackets for SCSS)

[saper]

Something to consider is how we track tests that we think conform well to the style guide versus those that do not.

Maybe some conformance level indicator in the options.yml metadata? Something like :need-style: or :style: maybe with a guide version number.

A tool proposed in #1517 could extract and report them.

[Awjin]

Something to consider is how we track tests that we think conform well to the style guide versus those that do not. [...] One option would be to design an ideal directory structure in this issue, but continue to only actually put files into it as they go through the style-guide-ification process.

I like this option.

Maybe some conformance level indicator in the options.yml metadata? Something like :need-style: or :style: maybe with a guide version number.

Since we have to inspect each file to determine if it's compliant, I'd prefer to put all old specs into a non-conformant bucket. We could pull each spec out as it gets style-guide-ified. The repo would look like

spec/
  [ideal diretory structure]/
  non_conformant/
    basic/
    errors/
    [...]

with the goal of emptying non_conformant/. This could also reduce confusion for new contributors.

Does it make sense to make scope/ a subdirectory of variables/? A lot of scoping logic (other than reassignment) applies to mixins and functions as well.

Probably not. The scoping tests can be spread out across all constructs that affect scope.

Should selectors/ be style_rules/, to match the general principle of categorizing specs by the statement-level construct that they refer to?

SGTM.

What are you envisioning would fall under "syntax" as opposed to any other category? It seems like kind of a catch-all.

It's definitely a catch-all for specs that test syntax errors but don't quite fit into a feature area. On second thought, I want to keep this as parser/, and put the following specs in it:

• comments/
• delimiters/
• indentation/
• interpolation/

The following specs that currently exist in parser/ can just be error specs in their corresponding features:

• arglists
• malformed_expressions

Second draft:

core_functions/
css/
directives/
non_conformant/
operators/
parser/
  comments/
  delimiters/
  indentation/
  interpolation/
style_rules/
  nesting/
  selectors/
  variables/
values/

[nex3]

That mostly looks good, but I have a few more questions:

• Is the idea that parser/interpolation just tests the general edge-cases of interpolation, but specs covering interpolation in specific contexts (selectors, declarations, etc) live with those features?

• Why is variables under style_rules?

• Should we have style_rules/declarations?

• It seems like there's some ambiguity about what should and shouldn't go in css. parser/comments, parser/delimiters, and some chunk of style_rules, values, and directives all cover features that exist in CSS itself. How should we draw that line?