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

Add parsing error for empty declaration value #957

Merged
merged 2 commits into from
Mar 18, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Mar 17, 2015

Fixes #945 - Adds a function to output errors in the same way as ruby sass does, which IMO is the final goal. IMO it doesn't matter too much if error messages are not consistent in libsass for the time beeing, so we can gradually change all errors to give the same output (IMO this is easier once we actually can test errors with spec tests).

@am11
Copy link
Contributor

am11 commented Mar 17, 2015

We would probably need to prevent sassc from prompting error on stderr, but redirect to stdout (2>&1)?

Print context from actual parsed source as ruby sass does!
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) to 80.53% when pulling 4e5472e on mgreter:bugfix/issue_945 into 4217540 on sass:master.

@mgreter
Copy link
Contributor Author

mgreter commented Mar 17, 2015

Holy moly, AppVeyor returned before Travis-CI 😲 @am11 IMO it would be more usefull to split the stderr into its own file (2>output.stderr). Basically we expect the same error for all four output styles, so if the test runner sees such a stderr file, it only has to do one test (at least that's how I probably would implement it today).

@am11
Copy link
Contributor

am11 commented Mar 17, 2015

Now Sass org is using AppVeyor FOSS Pro account (since last night), so we can experiment with it with much faster pace. 😉 😎 ☀️
There was no way we would wait for hours to get the CI job done!

@mgreter
Copy link
Contributor Author

mgreter commented Mar 18, 2015

@xzyfer This could either be 3.2 or 3.3 IMO, but since it fixes one bug, we may want to include it in 3.2 anyway. I could also remove the new error output and only move that to 3.3 (I added some more text to the first post in this PR). IMO we should at least address the error itself!

@xzyfer
Copy link
Contributor

xzyfer commented Mar 18, 2015

I think this is fine to ship in 3.2. it'll give us a chance to fix any gotchya in 3.3. 🚢

@mgreter mgreter added this to the 3.2 milestone Mar 18, 2015
mgreter added a commit that referenced this pull request Mar 18, 2015
Add parsing error for empty declaration value
@mgreter mgreter merged commit bbfb15e into sass:master Mar 18, 2015
@mgreter mgreter deleted the bugfix/issue_945 branch March 18, 2015 23:58
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 this pull request may close these issues.

It does not error when a property-value is missing
4 participants