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

Encapsulate sass_interface.h to allow for cleaner C API and allow room for future additions #79

Closed
bitc opened this issue Dec 12, 2012 · 5 comments
Assignees

Comments

@bitc
Copy link
Contributor

bitc commented Dec 12, 2012

I propose to encapsulate the structs in sass_interface.h behind an interface of getter and setter functions.

For example, instead of doing:

struct sass_context* ctx = sass_new_context ();
ctx->source_string = "a { color: blue}";

One would instead do:

sass_context* ctx = sass_new_context ();
sass_context_set_source_string("a { color: blue}");

The actual structs will no longer be exposed. This has the following advantages:

  • It allows the internal structures to be modified, or fields added to them, without breaking the source or binary API.
  • It makes the API clearer about which fields are read only, and enforces this.
  • It (arguably) will allow for easier implementation of bindings to other languages, since there is no more dealing with C structs, just plain functions that take ordinary data types (int, char*)

While we're at it, I really don't like the method of using a single colon-separated string for the include_paths (especially given that one has to detect if the platform is windows and use a semi-colon instead). I think instead of this:

options.include_paths = "libs/colors:sass:/usr/include/sass";

The following is a lot cleaner:

sass_options_add_include_path("libs/colors");
sass_options_add_include_path("sass");
sass_options_add_include_path("/usr/include/sass");

I realize that these changes will break existing code that uses libsass, but I believe that these changes would be a clear improvement, and better sooner than later, while libsass is still relatively young. Plus, this encapsulation will allow for future improvements and additions to the API without breaking code.

I am willing to write the code for these changes and submit a pull request.

@akhleung
Copy link

Sounds like a good idea in general ... just give me a few days to think about some of the specifics. It might be good to continue to make original C struct available (but mark it as deprecated) so that users don't have to change anything, and the API functions can either use a new struct type, or access the C++ classes directly.

@HamptonMakes
Copy link
Member

I'd consider this for a future release. Would definitely break existing integrations, but we'd happily take this patch.

@mgreter
Copy link
Contributor

mgreter commented Nov 17, 2014

This should be fixed with the merge of #635!
There is a wiki page with some API Documentation.

@mgreter mgreter closed this as completed Nov 17, 2014
@drewwells
Copy link
Contributor

How do you free the context with the new interface? The code says that the old interface already does this, so.... should I include both interfaces to use this feature?

@mgreter
Copy link
Contributor

mgreter commented Nov 18, 2014

I had to rename the free functions to delete to avoid name clashes with the old interface. To free the context you simply need to call sass_delete_file_context or sass_delete_data_context. You actually should never use both interfaces at the same time, since the data structures are not compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants