Relax compact error to warning

[mgreter]

This relaxes the error to a warning. Otherwise the function cannot be used by user-code. It will now just spit out a warning, but behavior in output matches ruby sass. Makes a few spec tests to pass now:

• libsass/eq
• libsass/compact

[coveralls]

Coverage increased (+0.0%) to 80.63% when pulling 3f1c048 on mgreter:lower-compact-to-warning into f82a41b on sass:master.

[xzyfer]

My preference is we simply just drop this error/warning completely. Mentioning it as a breaking change should be enough IMO. I don't see the benefit in keeping this code debt around.

[mgreter]

How about relaxing the error in 3.2.1 to a warning and removing it completely in 3.3.0 (3.2.2)?

[xzyfer]

Sure thing!

[xzyfer]

Thinking about this does it makes sense to go form error -> warning -> gone?

Ship this in 3.2.0 instead?

[mgreter]

When did we introduce that error message (was a version already released with that message included)? IMO it does make sense to make people aware that the behavior has changed! But personally I don't really have a preference here ...

[xzyfer]

I believe it was introduced in 3.2.0.

IMO error -> warning -> gone make no sense. Either

• warn -> error -> gone
• error -> gone

[mgreter]

I believe it was introduced in 3.2.0.

So we haven't even released the "deprecated" error yet. Problem is that the error will disallow the use of custom functions named "compact". But we IMHO still want to inform the user that his code will no longer work the way it did. The errors seems to be a bit too much, but warnings might get overlooked by users. So IMHO it makes sense to first use an error (failing hard), then emit a soft warning and finally ignore it altogether. This should make a nice transition for users that use this function, but an error also disallows any "polyfill" implementation, so maybe going with a warning is the best option?

Actually I don't see why "warn > error > gone" would make any sense at all?

[xzyfer]

So we haven't even released the "deprecated" error yet.

Correct.

warnings might get overlooked by users

I expect they'll notice when their code doesn't work :)

Actually I don't see why "warn > error > gone" would make any sense at all?

warn > error > gone and error -> gone are patterns used by the Ruby Sass team.

an error also disallows any "polyfill" implementation

This makes erroring a non-starter for me. I had assumed native (libsass) functions would have the lowest priority in the function resolution process.

maybe going with a warning is the best option

I think warning -> gone is the best option here unless we change the resolution order of function calls.

[xzyfer]

Would not incorrectly warn users even if they have defined their own compact function? Could we move the warning into the functions.cpp?

[mgreter]

I also saw that, but wasn't sure how to better address it. But we surely can move that to some more appropriate place. IMO we might be able to add a specific if clause just before internal functions are resolved, and if the name is compact emit this warning!?

[xzyfer]

@mgreter I think I have misunderstood the issue here.

Otherwise the function cannot be used by user-code

I thought you meant the current master made it impossible for users to define their own compact method. This does not appear to be the case as this code execute fine.

@function compact() {
  @return foo; }

foo {
  bar: compact(); }

Makes a few spec tests to pass now

The specs you've mentioned are invalid and should be deleted as compact has never been a supported function in Ruby Sass - sass/sass-spec#326

---

I think we should leave the current behaviour and remove the error completely 3.2.1/3.3. Thoughts?

[coveralls]

Coverage increased (+0.01%) to 80.28% when pulling 58ffae1 on mgreter:lower-compact-to-warning into 9ad775f on sass:master.