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

Update the README #1353

Merged
merged 2 commits into from
Mar 7, 2019
Merged

Update the README #1353

merged 2 commits into from
Mar 7, 2019

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Mar 2, 2019

It now explains how to run specs for each major implementation, how
specs are laid out, and the available options. It also provides some
guidance for where to put new specs.

@nex3 nex3 requested review from nschonni and nshahan March 2, 2019 03:03
@nex3
Copy link
Contributor Author

nex3 commented Mar 2, 2019

FYI @xzyfer, @mgreter, @glebm

It now explains how to run specs for each major implementation, how
specs are laid out, and the available options. It also provides some
guidance for where to put new specs.
# If you already have a clone of the Dart Sass repo, you can use that instead.
git clone https://github.com/sass/dart-sass
(cd dart-sass; pub get)
export DART_SASS_PATH=`pwd`/dart-sass
Copy link
Contributor

@nschonni nschonni Mar 2, 2019

Choose a reason for hiding this comment

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

I think $pwd is more portable than backticks, but i'm not a bash expert
Edit: maybe it's $(pwd) https://github.com/sass/libsass/blob/5c2501eaa115aafafb254fa4f3522efa0ee14edb/script/ci-build-libsass#L12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Backquotes should work for any sh-compliant shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think I was coming more from https://github.com/koalaman/shellcheck#style but it looks like that s a style rather than bug rule koalaman/shellcheck#364 (comment)

@nex3 nex3 mentioned this pull request Mar 4, 2019
5 tasks
Copy link

@nshahan nshahan left a comment

Choose a reason for hiding this comment

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

A few comments but looks great!

README.md Outdated
:path => "..."` then the Sass unit tests will run against your
development version of sass-spec.
HRX archives can contain directories, which means that they can also a single
file can define multiple different This allows us to write many specs for the
Copy link

Choose a reason for hiding this comment

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

Unfinished sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, this whole paragraph is a mess. Done.

* An `error` file, in which case the spec asserts that the Sass implementation
prints the error message to standard error and exits with a non-zero status
code when it compiles the input. These specs are known as "error specs".

Copy link

Choose a reason for hiding this comment

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

Maybe add a short callout with link to the "implementation-specific-expectations" section here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nex3 nex3 requested a review from nschonni March 5, 2019 02:44
@nex3 nex3 merged commit b10a4aa into master Mar 7, 2019
@nex3 nex3 deleted the readme branch March 7, 2019 01:56
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