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

Tests and cleanup #11

Merged
merged 8 commits into from
Aug 28, 2014
Merged

Tests and cleanup #11

merged 8 commits into from
Aug 28, 2014

Conversation

ilanbiala
Copy link
Contributor

No description provided.

@ilanbiala ilanbiala mentioned this pull request Aug 23, 2014
@ilanbiala ilanbiala changed the title Develop Tests and cleanup Aug 23, 2014
@ilanbiala
Copy link
Contributor Author

@keithamus @nschonni maybe you guys can give me some thoughts on the second test I made. I added supertest for HTTP request testing, but I'm not sure how to handle the routing for the server. I'd like to add tests for the current state of the package before moving on and trying to make prefix working and allow separate folders.

@ilanbiala ilanbiala mentioned this pull request Aug 24, 2014
@keithamus
Copy link
Member

@ilanbiala Leave it to me this evening and I'll add to this PR, filling out the tests.

@ilanbiala
Copy link
Contributor Author

@keithamus I just merged in your changes, and all the tests pass, so I'm going to start working on different folders for middleware, but I'll do that in a separate PR. Is prefix: /prefix supposed to specify that the file should be located in /prefix/file-request-path.css instead of /file-request.path.css?


# Unix-style newlines with a newline ending every file
[*]
end_of_line = lf
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different from the upstream config https://github.com/sass/node-sass/blob/master/.editorconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't much different, but it's the editorconfig for this package, which is the same as node-sass's. That should be true, because it's the same org. and style-guide. By the way, it should be space, not spaces for indentation style.

Copy link
Member

Choose a reason for hiding this comment

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

For consistency we should keep it inline with upstream. Having said that upstream should have the end_of_line option, which it currently omits (see sass/node-sass#402)

consistent with upstream node-sass
@ilanbiala
Copy link
Contributor Author

Just modified the .editorconfig so it is consistent with node-sass's.

@keithamus
Copy link
Member

LGTM 👍

keithamus added a commit that referenced this pull request Aug 28, 2014
@keithamus keithamus merged commit bcd4536 into sass:master Aug 28, 2014
@keithamus
Copy link
Member

Thanks for your hard work on this @ilanbiala 😄

@ilanbiala ilanbiala mentioned this pull request Aug 28, 2014
Closed
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