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

Add test for sass/libsass#2786 #1370

Merged
merged 1 commit into from
Apr 4, 2019
Merged

Add test for sass/libsass#2786 #1370

merged 1 commit into from
Apr 4, 2019

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Apr 4, 2019

Tests for sass/libsass#2786 fixed by sass/libsass#2860

[skip libsass]

glebm added a commit to glebm/libsass that referenced this pull request Apr 4, 2019
@nex3
Copy link
Contributor

nex3 commented Apr 4, 2019

I know the style guide hasn't officially landed yet, but it's still a good idea to follow it for new specs.

@glebm
Copy link
Contributor Author

glebm commented Apr 4, 2019

@nex3 Reading it now, is there anything in particular that stands out?

@@ -0,0 +1,19 @@
<===> input.scss
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Read the style guide, should this then be: error/input.scss?


-----------------------^

<===> error-dart-sass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The style guide says "use dart sass output as canonical" but here Ruby and libsass have a much better error message. I've left a question on the style guide about this.

@glebm
Copy link
Contributor Author

glebm commented Apr 4, 2019

@nex3 [skip libsass] didn't seem to work here

@nex3
Copy link
Contributor

nex3 commented Apr 4, 2019

Reading it now, is there anything in particular that stands out?

[skip libsass] didn't seem to work here

Oh, I thought you were a repo collaborator, but I guess not. It's a lot flakier for non-collaborators because they don't have access to the encrypted @sassbot GitHub credentials to read the commit message. I've sent you an invite to the sass-spec team.

@glebm
Copy link
Contributor Author

glebm commented Apr 4, 2019

Specs should be organized by which part of the language they're testing (this is currently in the README rather than the style guide, but it should probably be in the latter)

+1, everything a contributors need to know before submitting a new spec should be in one place.

Oh, I thought you were a repo collaborator, but I guess not

The mechanism in the libsass repo works even without that but I guess it uses public GitHub API (which may run out of quota with lots of PRs).

@glebm
Copy link
Contributor Author

glebm commented Apr 4, 2019

Thanks for the detailed feedback, I've addressed all the comments.

@glebm glebm force-pushed the supports branch 2 times, most recently from 812cb78 to 6c70f80 Compare April 4, 2019 23:33
spec/directives/supports.hrx Outdated Show resolved Hide resolved
@nex3 nex3 merged commit 53c8f41 into sass:master Apr 4, 2019
glebm added a commit that referenced this pull request Apr 4, 2019
@glebm glebm deleted the supports branch April 5, 2019 00:01
xzyfer pushed a commit to sass/libsass that referenced this pull request Apr 5, 2019
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.

2 participants