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
Implement Index and a default file if directory given in import #220
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! There's one behavioral change I'd like to see, though.
lib/src/importer/utils.dart
Outdated
? _tryPath(path) | ||
: _tryPathWithExtensions(path); | ||
return file != null ? file : _tryPathAsDirectory(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a .scss
or .sass
extension in the import should preclude looking for an index file.
test/node_api/importer_test.dart
Outdated
includePaths: [sandbox], | ||
importer: allowInterop(expectAsync2((_, __) {}, count: 0)))), | ||
equalsIgnoringWhitespace('a { b: c; }')); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index file support is more a part of the language than the API; it's an aspect of what it means for a filesystem import will mean. Which is to say, I think the sass-spec tests are sufficient and we don't need implementation-specific tests like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just waiting on the other PRs and the CLA signature.
I signed it! |
It looks like the CLA bot can't find your signature... is it associated with your GitHub account and the email you used for these commits? |
Ah bugger I was advised to sign with my work email which is the secondary email in GitHub |
If you run |
I signed it! |
I'm still not seeing that email in the list of signers... I'm going to contact internal support to see if I can get this sorted out. Sorry for the trouble! |
Sounds like we just have to wait a couple days for the lawyers to approve Marks & Spencer's paperwork. |
No worries. Its the first time I or M&S have contributed to any big open source project AFAIK |
I signed it! Hoping it worked this time 🤞 |
Looks like not yet 😕. I've poked the internal folks again. |
Yeah, it keeps getting rejected saying our executive isn't high enough Head of Engineering just got rejected. We are working with our UK Google contact to figure out how high we have to go. |
I signed it! |
I do see the CLA internally now, but apparently googlebot still isn't happy? I'm not sure what's going on... I'm sorry this process is being such a pain 😭. I really appreciate you sticking it out. |
Can you force skip the bot? |
Unfortunately, I can't. I've reached out to the internal team again to see if there's something else you need to do. |
I signed it! |
CLAs look good, thanks! |
Ref to: sass/sass#2456
I've not done Dart before so any help will be accepted.
See sass/sass-spec#1206