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

add compile_string() for compiling strings to css #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hhaensel
Copy link

This PR fixes #2.

  • modify sass_make_data_context() to always copy a Julia AbstractString to sass in order to prevent segfaults
  • add compile_string() with corrected sass_make_data_context()
  • add respective tests

The following code example from sass helped me to develop this PR
https://github.com/sass/libsass/blob/master/docs/api-context-example.md

Not sure whether this API is the optimal solution. One could think of having only compile() with multiple dispatch, or defining compile_sass(), compile_scss() with the respective setting of is_indented_syntax_src.

compile_sass(data; kwargs...) = compile_string(data; compile_sass = true, kwargs...)
compile_scss(data; kwargs...) = compile_string(data; compile_sass = false, kwargs...)

@piever Happy to hear what you think.

@piever
Copy link
Owner

piever commented May 18, 2023

Thanks for the PR! In terms of API, the second approach makes the most to me. There isn't really a custom type for filepaths in julia, so having

compile_file # infers format from extension
compile_sass # takes a string (or maybe even an `IO` object)
compile_scss # takes a string (or maybe even an `IO` object)

makes the most sense to me. compile_string can remain as an internal function to avoid code duplication.

@hhaensel
Copy link
Author

Some more thoughts;

  • we could have some automatic format detection of sass/scss in compile_string
  • I have not included source_maps in compile_string, because I thought that this would be a rather strange use case to work with string css but source map files. If you think differently we should re-include those parts (not much of a deal).

@hhaensel
Copy link
Author

hhaensel commented Jun 5, 2023

@piever Shall we merge it as is or do you think that more modifications should be done?

@piever
Copy link
Owner

piever commented Jun 7, 2023

LGTM! Still, it could be nice to add a small example in the README, as otherwise one can only found out about Sass.compile_sass and Sass.compile_scss by reading the source code.

@codecov-commenter
Copy link

Codecov Report

Merging #26 (d680ac4) into master (64a2c2a) will increase coverage by 19.38%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master      #26       +/-   ##
===========================================
+ Coverage   21.79%   41.17%   +19.38%     
===========================================
  Files           6        6               
  Lines         156      187       +31     
===========================================
+ Hits           34       77       +43     
+ Misses        122      110       -12     
Impacted Files Coverage Δ
src/context.jl 68.75% <100.00%> (+36.31%) ⬆️
src/julian_api.jl 78.26% <100.00%> (+15.29%) ⬆️

... and 1 file with indirect coverage changes

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.

Compile from string
3 participants