search

sensational / sassphp

PHP bindings to libsass - fast, native Sass parsing in PHP!
Other
230 stars 40 forks source link

Change style constants to SASS_OUTPUT_* #8

Closed chrinor2002 closed 9 years ago

chrinor2002 commented 9 years ago

Currently the style constants are setup to be SASSSTYLE* as defined in sass_interface.h. These should be changed to SASSOUTPUT* to be in a single consistent format.

Also the names of the comments in the tests for the getter/setter are SASSOUTPUT*.

pilif commented 9 years ago

Thanks a lot. In principle, I agree with you.

However, I wonder how widely used this already is as this is, strictly speaking, a backwards compatibility break.

Maybe we need an alias, or at least some userspace file people using the existing library can include in order for their code to continue working. Let me think a bit about this.

chrinor2002 commented 9 years ago

Do you mean in libsass? or in sassphp? From the sassphp perspective it should not affect any code as constants are identical on the class, AND it just so happens that the numeric values of each constant from libsass are identical. I would be interested to know what one is "correct" from a libsass perspective though.

Cheers!

pilif commented 9 years ago

ah. I understand. I should have taken a more detailed look. I thought you wanted to change the constants in PHP.

Anyways - I wonder what's the correct thing to do here: there's both sass_interface.h and sass.h.

Back then, there was only sass_interface.h, so that what I used.

sass.h is more advanced, so I guess that's the way to go in the future. However, the extension is currently using the declarations in sass_interface.h and following that convention, we should probably stick to the defines in there, which is SASS_STYLE_*

I'll try to find out more about which of the two header files to use when

pilif commented 9 years ago

Update: I noticed that the SASS_STYLE_* constants have been removed in a later commit, but as we're using the 3.0.2 release right now, I think that SASS_STYLE_* still is the correct thing to use as this is what's declared in the one header file the release tells us to use.

As we update to a later release, this will have to change.

Leaving this open as a reminder to do so.

chrinor2002 commented 9 years ago

I couldn't agree more.

chrinor2002 commented 9 years ago

Looks like the SASS_OUTPUT constants were removed in the latest versions. So we will probably have to go back to the SASS_STYLE constants :( unfortunately that means the PHP constant STYLE_FORMATTED => SASS_OUTPUT_FORMATTED will need to eventually be removed as there are only 4 STYLE constants: ./sass.h: SASS_STYLE_NESTED, ./sass.h: SASS_STYLE_EXPANDED, ./sass.h: SASS_STYLE_COMPACT, ./sass.h: SASS_STYLE_COMPRESSED

cough my bad. We could probably close this has wont fix or invalid at this point.