Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make linefeed/indent configurable #787

Merged
merged 3 commits into from
Jan 2, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Dec 30, 2014

Added the necessary options to the context and tried to implement the output for newlines and indentations accordingly. In reference to #248 and #752. Please note that you need to provide const char* for these options (libsass will not make a copy, since we expect implementers to pass static strings, or strings that the implementor will free after the context gets out of scope)!

@mgreter mgreter added this to the 3.next milestone Dec 30, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling 1a937f2 on mgreter:feature/configurable-linefeed into 3b0feb9 on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented Dec 30, 2014

👍

@am11
Copy link
Contributor

am11 commented Dec 30, 2014

Super fantastic. You nailed it! 😸 👍

@mgreter mgreter changed the title [WIP] Make linefeed/indent configurable Make linefeed/indent configurable Jan 1, 2015
@am11
Copy link
Contributor

am11 commented Jan 1, 2015

Will this be included in v3.1.0?

@xzyfer
Copy link
Contributor

xzyfer commented Jan 1, 2015

No. It will be in the first patch release 3.1.1 which will be out shortly
after 3.1.0 stable. I expect that release will be in the next week or two
since we already have PR scheduled for that release.
On 2 Jan 2015 05:00, "Adeel Mujahid" notifications@github.com wrote:

Will this be available for v3.1.0?


Reply to this email directly or view it on GitHub
#787 (comment).

@am11
Copy link
Contributor

am11 commented Jan 1, 2015

Any ETAs for The v3.1.0 RTW ?

@xzyfer
Copy link
Contributor

xzyfer commented Jan 1, 2015

Within the next day I'd say ;)

(edited)

@am11
Copy link
Contributor

am11 commented Jan 2, 2015

Cool!

@xzyfer, I invited you on gitter SASS organization channel (https://gitter.im/sass/~chat#), but you aren't enrolled in the organization. :(

@xzyfer
Copy link
Contributor

xzyfer commented Jan 2, 2015

We have one final blocker for 3.1 IMO #697 (comment)

@mgreter mgreter self-assigned this Jan 2, 2015
@mgreter mgreter modified the milestones: 3.2, 3.next Jan 2, 2015
@mgreter mgreter force-pushed the feature/configurable-linefeed branch from 1a937f2 to 2a1ff23 Compare January 2, 2015 20:55
@mgreter mgreter force-pushed the feature/configurable-linefeed branch from 2a1ff23 to 706b6df Compare January 2, 2015 21:01
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling 706b6df on mgreter:feature/configurable-linefeed into 1ab48ec on sass:master.

mgreter added a commit that referenced this pull request Jan 2, 2015
@mgreter mgreter merged commit 20685b4 into sass:master Jan 2, 2015
@am11
Copy link
Contributor

am11 commented Mar 23, 2015

@mgreter, is there a test case for indent and linefeed options in perl or spec?

@mgreter
Copy link
Contributor Author

mgreter commented Mar 23, 2015

Definitely not in the specs (we cannot test different options, beside output style) and also not in perl-libsass (just looked). Why? Are you having problems with them? If so it should not take much time to add a test case to perl-libsass?

@am11
Copy link
Contributor

am11 commented Mar 23, 2015

Yes, actually it is emitting garbage characters. I tested with C-string and then std::string initializer (.c_str). On first runs, it totally ignores the indent setting, but respect the linefeed. But on second run and onward, it is emits some garbage characters with indent, which is forcing libsass to emit @charset "UTF-8".

@mgreter
Copy link
Contributor Author

mgreter commented Mar 23, 2015

I just added the options to perl-libsass and it works as expected, so not sure what is going wrong with your implementation. As a reference you can find the PR for my work in sass/perl-libsass#10

@am11
Copy link
Contributor

am11 commented Mar 23, 2015

Thanks! I will push my changes shortly.

@am11
Copy link
Contributor

am11 commented Mar 23, 2015

Here is the commit: am11/node-sass@fd77d18 (updated)
I have added two options in API for indentation, indentWidth (int) and indentType (space or tab). Based on that, I am constructing a cstring and passing it to libsass. In return, I am getting faulty characters after the linefeed (at indent character position).

@am11
Copy link
Contributor

am11 commented Mar 23, 2015

Here is how the failing test look like: https://travis-ci.org/am11/node-sass/jobs/55564050#L1380. Over on CIs it is happening intermittently. But locally I can reproduce it persistently.

@am11
Copy link
Contributor

am11 commented Mar 24, 2015

@mgreter, finally it worked! sass/node-sass#792

(C)Strings have tendency to give us hard time..

@mgreter mgreter deleted the feature/configurable-linefeed branch April 6, 2015 17:14
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 706b6df on mgreter:feature/configurable-linefeed into * on sass:master*.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants