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

Remove placeholder for Sass lang version #2077

Merged
merged 1 commit into from
May 8, 2016
Merged

Remove placeholder for Sass lang version #2077

merged 1 commit into from
May 8, 2016

Conversation

am11
Copy link
Contributor

@am11 am11 commented May 7, 2016

Based on the commentary in PR #2021, there is no need to define
a placeholder for language version in version.h.in file. Instead we
define the constant in version.h for non-gcc compilers.

With this change MSVC and clang compiled outputs will not show [NA]
anymore.

@am11
Copy link
Contributor Author

am11 commented May 7, 2016

cc @mgreter

@coveralls
Copy link

coveralls commented May 7, 2016

Coverage Status

Coverage remained the same at 79.286% when pulling abd6807 on am11:master into 3b1e9ff on sass:master.

Based on the commentary in PR sass#2021, there is no need to define
a placeholder for language version in `version.h.in` file. Instead we
define the constant in `version.h` for non-gcc compilers.

With this change MSVC and clang compiled outputs will not show [NA]
anymore.
@am11 am11 changed the title Define Sass lang version just once Remove placeholder for Sass lang version May 7, 2016
@coveralls
Copy link

coveralls commented May 7, 2016

Coverage Status

Coverage remained the same at 79.286% when pulling 01f370a on am11:master into 3b1e9ff on sass:master.

@mgreter
Copy link
Contributor

mgreter commented May 8, 2016

LGTM, but as I wrote earlier, It doesn't serve much use for libsass ATM, until we start to actually implement different language versions. Although we are pretty close to sass 3.4, the underyling code is really not ready to move on to 3.5 atm. (let aside suporting multiple versions of the language) There are just too many places where implementiation differs from sass implementation. We do try to get these in line with ruby sass, but here are a lot of subtile differences where I personaly don't know how to implement the behavior 100% correctly in libsass (even after looking at the sass/ruby code). So for these cases we either need someone who knows the sass specs inside out and can provide fixes for the C++ code. But for now I think we'll be stuck with sass 3.4 for quite a long time ... so LGTM!

@xzyfer
Copy link
Contributor

xzyfer commented May 8, 2016

@mgreter feel free to ask me in these cases. I know the language spec very well and know my way around the Ruby code base.

@xzyfer xzyfer added this to the 3.3.7 milestone May 8, 2016
@xzyfer
Copy link
Contributor

xzyfer commented May 8, 2016

These failures are strange.

@xzyfer
Copy link
Contributor

xzyfer commented May 8, 2016

Rebuilding the failed build to see if it's a blip.

@xzyfer
Copy link
Contributor

xzyfer commented May 8, 2016

CI looks better \o/

@xzyfer xzyfer merged commit e80b63d into sass:master May 8, 2016
@mgreter
Copy link
Contributor

mgreter commented May 8, 2016

@xzyfer #1392 for example ... I don't know why this is different than in ruby sass ...

@xzyfer xzyfer modified the milestones: 3.3.7, 3.4 Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants