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

include_path vs include_paths #12

Closed
mortenpi opened this issue Nov 24, 2019 · 3 comments · Fixed by #14
Closed

include_path vs include_paths #12

mortenpi opened this issue Nov 24, 2019 · 3 comments · Fixed by #14

Comments

@mortenpi
Copy link
Contributor

mortenpi commented Nov 24, 2019

For setting the include paths, the documentation

- `include_paths` (`AbstractString` or `AbstractArray{<:AbstractString}`)

disagrees with the implementation

:include_path => sass_option_set_include_path,

As libsass seems to have s at the end, I guess the implementation should be changed to :include_paths => sass_option_set_include_paths?

@piever
Copy link
Owner

piever commented Nov 24, 2019

If I remember correctly, the rationale was that the C API has include_path to pass one string (of either one path, or many "colon separated" paths), and include_paths to pass an array of strings. At least, that's what I understood from reading here. At the time I thought that this API was a bit odd.

Possible options:

  1. Stick to C API and have different keywords for one string or many
  2. Choose include_path for both one string or many
  3. Choose include_paths for both one string or many
  4. Allow either include_path or include_paths for both one string or many

Which makes the most sense to you? I suspect 3 is the Julian way, as in the case of "one or many" the default is the plural. OTOH it may be not ideal to have include_paths and plugin_paths, but input_path and output_path.

@mortenpi
Copy link
Contributor Author

Sorry for forgetting to reply to this. My feeling is that since the keyword is directly related to the sass_option_set_include_path function, it would make sense to keep it include_path.

I didn't read the C code, but looking at the C API, my hunch is that include_paths is just an internal helper variable and is kept synced with include_path. As far as I can tell, include_path can contain either a single path or many (colon-separated) and in either case you should set it with sass_option_set_include_path.

mortenpi added a commit to mortenpi/Sass.jl that referenced this issue Feb 13, 2020
This is only a documentation change, bringing the docs in line with both
the Julia implementation and the C API.

Fix piever#12.
@piever
Copy link
Owner

piever commented Feb 14, 2020

Makes sense, I also prefer changing the documentation rather than the implementation. Thanks for making the PR!

piever pushed a commit that referenced this issue Feb 14, 2020
This is only a documentation change, bringing the docs in line with both
the Julia implementation and the C API.

Fix #12.
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 a pull request may close this issue.

2 participants