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.

Sign up for GitHub

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

Jump to bottom

Updated to handle writing multiple files to folder #13

Merged
merged 3 commits into from
Jun 6, 2015
Merged

Updated to handle writing multiple files to folder #13

merged 3 commits into from
Jun 6, 2015

Conversation

deanhume
Copy link
Contributor

@deanhume deanhume commented Jun 5, 2015

As discovered in issue #12, when trying
to write multiple files to a folder, the write fails because it assumes that the path is a file and not a folder.

I have updated the code to check first if the destination is a folder and update accordingly. See this
issue on SO for more information.

@deanhume
Updated to handle writing multiple files to folder
c57ac9a 
As discovered in issue
[12](bezoerb#12), when trying
to write multiple files to a folder, the write fails because it assumes
that the path is a file and not a folder.

I have updated the code to check first if the destination is a folder
and update accordingly. See [this
issue](http://stackoverflow.com/a/20417119/335567) on SO for more
information.
@deanhume deanhume mentioned this pull request Jun 5, 2015
Open
@bezoerb
Copy link
Owner

bezoerb commented Jun 5, 2015

Thanks, but it seems you broke the existing tests.
Tests need to be green and we need a new test that covers your changes before this could be merged

@deanhume
Copy link
Contributor Author

deanhume commented Jun 5, 2015

The mocha tests are failing, but I have no idea why as I haven't changed anything to do with the way the files are processed. the generated CSS does appear to be different to the expected CSS. Does this have to do with a newer version of critical? Any ideas?

@bezoerb
Copy link
Owner

bezoerb commented Jun 5, 2015

looks like a newer version of clean-css. We had the same issue over at critical. Could you provide a testcase for your changes? I'll check the different css this evening.

@deanhume
Copy link
Contributor Author

deanhume commented Jun 5, 2015

Test case added. The tests are still failing due to the clean-css issue, but other than that it should be all good. Let me know if you need any updates! Thanks

@bezoerb bezoerb merged commit 2d60013 into bezoerb:master Jun 6, 2015
bezoerb added a commit that referenced this pull request Jun 6, 2015
@bezoerb
Added test & additional check for pr #13
ee8e121
@bezoerb
Copy link
Owner

bezoerb commented Jun 6, 2015

made some small modifications ;)
Thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@deanhume @bezoerb