Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Added renderFile function #285

Merged
merged 2 commits into from
Apr 16, 2014
Merged

Added renderFile function #285

merged 2 commits into from
Apr 16, 2014

Conversation

simonexmachina
Copy link
Contributor

Handles writing the CSS and sourceMap to disk. Let me know if you like this and I'll add the docs and tests.

@keithamus
Copy link
Member

It would be great to see some tests for this.

@simonexmachina
Copy link
Contributor Author

Cool, will do. Just wanted to see if you were open to adding this first
On 12 Apr 2014 07:45, "Keith Cirkel" [email protected] wrote:

It would be great to see some tests for this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/285#issuecomment-40256414
.

@simonexmachina
Copy link
Contributor Author

I've added tests and docs. Also, I moved mocha to devDependencies.

I've added code to prepareOptions that defaults sourceComments to 'map' if a sourceMap is requested.

@keithamus
Copy link
Member

Mocha shouldn't be in devDependencies because it is used to test that the pre-built binaries work upon install. See build.js. Could you change it back please?

@keithamus
Copy link
Member

A couple of notes about the actual PR though:

The tests actually write to the filesystem - to me this is a nono, we should stub the fs.* commands. Say for example adding Sinon and sinon.stub(fs, 'writeFile') followed by assert.ok(fs.writeFile.called). Primarily because A) the test can fail based on random reasons like permissions of the folder it writes into and B) if the test fails after write but before cleanup, you've just littered my filesystem.

Having said that - I'd like @andrew's opinion on this matter, especially as adding Sinon or similar will add a new dependency. So don't rush to add this until he's made his say.

@simonexmachina
Copy link
Contributor Author

Yeah that's the reason I didn't add it. Will wait for his thoughts. I'll
change back mocha too.

@adamyeats-zz
Copy link

I'd like to add a +1 for Sinon stubs cc/ @andrew

@nschonni
Copy link
Contributor

This is already be done through the CLI, any reason why this should be in the API?

@simonexmachina
Copy link
Contributor Author

Tools such as grunt-sass and broccoli-sass need to generate source maps and
access the module directly, not using the API. I could have implemented
this logic in broccoli-sass, but thought that it belonged in this module.​

@andrew
Copy link
Contributor

andrew commented Apr 14, 2014

@keithamus @aexmachina I don't mind adding another dependency, Sinon sounds like a good idea.

@nschonni I don't see why this shouldn't be available as an API as it could be helpful for things like #245

@simonexmachina
Copy link
Contributor Author

All done.

@andrew
Copy link
Contributor

andrew commented Apr 16, 2014

Looks good, I suspect the next release will be in a few weeks after I've got married.

andrew pushed a commit that referenced this pull request Apr 16, 2014
Added renderFile function
@andrew andrew merged commit 6d3bdcd into sass:master Apr 16, 2014
@andrew
Copy link
Contributor

andrew commented Apr 16, 2014

@aexmachina this broke the tests on master, can you take a look? https://travis-ci.org/andrew/node-sass/jobs/23110757

@simonexmachina
Copy link
Contributor Author

Yep, was a simple fix. Have pushed, do you want me to create another PR?

@andrew
Copy link
Contributor

andrew commented Apr 18, 2014

Yes please

@simonexmachina
Copy link
Contributor Author

Cool, have done.​

@andrew
Copy link
Contributor

andrew commented Apr 22, 2014

I just released this as part of v0.8.5: https://github.com/andrew/node-sass/releases/tag/v0.8.5

@nschonni nschonni added this to the v0.8.5 milestone Apr 22, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants