Skip to content

added comma separated imports#23

Merged
connorskees merged 4 commits intoconnorskees:masterfrom
JosephLing:comma-seperated-imports
Jul 24, 2020
Merged

added comma separated imports#23
connorskees merged 4 commits intoconnorskees:masterfrom
JosephLing:comma-seperated-imports

Conversation

@JosephLing
Copy link
Copy Markdown
Contributor

Added comma seperated imports and 2 unit tests for it :)

The only thing missing maybe is that haven't worked out how to change the span to pick up on the change in location of the error. For example:

Error: Expected string.
  |
1 | @import 'dog', 1;
  |         ^
  |
./..\mole\_css\main.scss:1:9

Comment on lines +145 to +152
if s.ends_with(".css")
|| s.starts_with("http://")
|| s.starts_with("https://")
{
list_of_imports.push(Stmt::Import(format!("\"{}\"", s)));
} else {
list_of_imports.append(&mut self._parse_single_import(&s, span)?);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part seemed duplicated, could split it into a function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the code for pattern matching/parsing a single import is duplicated but it is only done duplicated once. And as the code won't be needed to be reused anywhere else I am going to keep it simple and not split it up. However can do if anyone wants it split up :)

@connorskees
Copy link
Copy Markdown
Owner

Thanks for working on this!

With regard to span information, that is currently a major failing of grass. For simplicity, spans are currently untested. This is not currently a big deal, and it should be resolved during work on adding support for source maps. You are free to ignore them for now (unless they are particularly egregious).

It would be nice to have tests for a heterogeneous group of imports -- that is, a comma separated import containing both plain CSS and Sass imports.

A test for the output of trailing commas, e.g. @import "foo.css", "bar.css",,,; would also be useful.

@connorskees connorskees changed the title added comma seperated imports added comma separated imports Jul 24, 2020
@connorskees
Copy link
Copy Markdown
Owner

Thank you! Everything looks good to me.

Ideally in the future we will use a different parser for imports (rather than relying on value parsing, but still using try_parse_url), but for now this works good enough for what should be most realistic use cases.

@connorskees connorskees merged commit 454b7c2 into connorskees:master Jul 24, 2020
@JosephLing
Copy link
Copy Markdown
Contributor Author

Sweet, what would be a good area of the code to look into learning how to do that? (using a different parser than value parsing)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants