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

Fixes for sass_interface (C Bindings) #181

Merged
merged 13 commits into from
May 8, 2014
Merged

Fixes for sass_interface (C Bindings) #181

merged 13 commits into from
May 8, 2014

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Oct 22, 2013

Hi

I'm currently trying to get the latest Version of libsass to work with Perl CSS::Sass. Unfortunately I've hit a few obstacles on the way:

A) The current sass_interface does not seem to be in sync with the rest of the library
B) The sass_interface crashes if there is no image_path set. This could be solved by the perl implementation, but I guess it doesn't hurt to include this protection for all implementations. By the way, it was crashing with this message:

terminate called after throwing an instance of 'std::logic_error'
what(): basic_string::_S_construct null not valid

I hope this pull request is useful!

Best wishes
Marcel Greter

@mgreter
Copy link
Contributor Author

mgreter commented Dec 11, 2013

Hello

First I want to thank you all, specially akhleung for working hard on the @extend implementation, for the ongoing work. I see you already fixed the "alpha opacity" with a much shorter patch. I just would like to ask about some feedback about this pull request. As I see it was not yet closed, but did not get any comments at all.

There are a few bugs addressed in this pull request that havent yet been reported IMO. If you'd like I could also open issues for each one of these (like the "expression" bug, which is another IE specific thing)? We really would like to use libsass via our perl deployment!

A quick feedback would be greatly appreciated!

Kind regards
Marcel Greter

@akhleung
Copy link

Sorry, I think I overlooked this because I confused it with a different pull request (also regarding sass_interface) that was ultimately withdrawn. I'll go through this tomorrow morning and get it merged in. Thanks!

@leafo
Copy link

leafo commented Dec 17, 2013

+1, I want to have c function support from sass_interface

@mgreter
Copy link
Contributor Author

mgreter commented Feb 25, 2014

I rebased my patches to the latest libsass version here: https://github.com/mgreter/libsass/tree/rebase-02. Added another patch that enables the use of sourcemaps with data not directly loaded by libsass. I also updated my branch for the perl Module CSS::Sass to handle source maps.

@qdsang
Copy link

qdsang commented Apr 11, 2014

@mgreter Failed?

qdsang referenced this pull request in 2betop/fis-sass Apr 11, 2014
Has obviously been renamed from C_Function_Data
Libsass crashes if implementer does not set a default
option for image_path. Protect the sass_interface the
same way as SassC does.
Re-introduced cookie for c functions (pointer to anything).
Moved actual function registration right after the built
in functions are registered.
They are still usefull so why disable it?
Added clause to only produce it when enabled!
Convert all files with sass extension via sass2scss.
Also searches for files with sass extension.
@akhleung
Copy link

Thanks, I'll take a look and get this merged in!

@qdsang
Copy link

qdsang commented Apr 19, 2014

Always believe that something wonderful is about to happen.

@VinSpee
Copy link

VinSpee commented Apr 25, 2014

I AM SO STOKED RIGHT NOW

@mgreter
Copy link
Contributor Author

mgreter commented May 4, 2014

Would be really cool if this would be finally merged in! The pull request is now open for half a year and I tried to keep up with the master branch. I also added the official spec tests, which give the same results before and after adding my changes. It would allow us to release a new version for perl and I guess all others trying to use the c_interface will profit from it. I really would love to see it (at least partially) be merged in! Don't get me wrong, but It starts to feel a bit like working for the bin.

@akhleung
Copy link

akhleung commented May 4, 2014

I realize it's been a long time, but now that the sass2scss stuff is part of this pull request, I just want to review it carefully before I merge it. Please hold on for a few more days!

@mgreter
Copy link
Contributor Author

mgreter commented May 4, 2014

Thanks for the feedback! Otherwise I'll bump it up again in a few weeks :)

@abstracthat
Copy link

I could really use this merged so I can implement it as a Hexo (node static site generator) plugin ASAP. It's tricky using ruby in this environment so indented sass support would be fantastic!

akhleung pushed a commit that referenced this pull request May 8, 2014
Fixes for sass_interface (C Bindings)
@akhleung akhleung merged commit 85f7b2e into sass:master May 8, 2014
@kennethormandy
Copy link

✨ Woah! ✨

Great work @mgreter and @akhleung

@akhleung
Copy link

akhleung commented May 8, 2014

Credit goes to @mgreter; I just clicked the merge button!

@mgreter
Copy link
Contributor Author

mgreter commented May 8, 2014

Woohoo! What great news and I really hope I didn't break anything for anyone else! If I did I already appologise for that. And please excuse my last "rude" comment about working for the bin, but yeah, I myself need some kick in the ass sometimes to get things done :)

Anyway, IMO the only commit that I'm not too sure about is in 691e101. I've just now added a comment for the specific change which I'm not sure if it will behave correctly (it does not seem to break with any existing test).

Thanks @akhleung for the merge!

@bountysource-support
Copy link
Contributor

@mgreter don't forget to claim the bounty on this! https://www.bountysource.com/issues/1057273-sass-files-not-supported/claims

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.

8 participants