Regression with custom precision

[mgreter]

Regression introduced in e231c6c
Breaks feature introduced in 07e9ead

Best regards!

[akhleung]

Interesting ... I suppose I could try storing only the precision in the To_String visitor instead of the entire Context object....

[HamptonMakes]

What's the status here...? I'm confused.

[mgreter]

Just tested with perl bindings and it seems that custom precision is stil broken (nsams@e231c6c).

[mgreter]

Reversed the logic @akhleung proposed and added a status bool to context, to flag this situation. Feels hackish, but IMO still better than breaking custom precision. Hope it works as intended.

[HamptonMakes]

Remember though, we are targeting 3.4 which I believe doesn't actually allow this. :/

On Mon, Oct 27, 2014 at 5:17 PM, Marcel Greter notifications@github.com
wrote:

Reversed the logic @akhleung proposed and simply added a status bool to context, to flag this situation. Feels hackish, but IMO still better than breaking custom precision. Hope it works as intended.

Reply to this email directly or view it on GitHub:
#364 (comment)

[mgreter]

Sorry, I dont really understand what you mean? Is custom precision not a feature of sass 3.4?

[xzyfer]

According to the changelog the --precision flag was introduces in 3.1.8 and defaulted to 3. In 3.2.0 it was defaulted to 5. Custom precisions via the --precision flag is still in a thing in ruby sass master.

[HamptonMakes]

I swear I was reading a thread on sass that discussed that... let me look around. Ignore me until I find it as, it could have been a threaded fever dream.

On Mon, Oct 27, 2014 at 7:48 PM, Michael Mifsud notifications@github.com
wrote:

According to the changelog the --precision flag was introduces in 3.1.8 and defaulted to 3. In 3.2.0 it was defaulted to 5. Custom precisions via the --precision flag is still in a thing in ruby sass master.

Reply to this email directly or view it on GitHub:
#364 (comment)

[mgreter]

Closing this issue as it is fixed in 12dbd03

[xzyfer]

@mgreter there are reports that this change set is affecting compiling on Windows #586, sass/node-sass#487 (comment).

I'm also see compilation warnings on clang

context.cpp:66:5: warning: field 'subset_map' will be initialized after field '_skip_source_map_update' [-Wreorder]
    subset_map              (Subset_Map<string, pair<Complex_Selector*, Compound_Selector*> >()),
    ^
1 warning generated.

Could you please look into this issue?

/cc @am11

[mgreter]

I saw this warning too. I have re-ordered it in my current WIP branch: mgreter@66e1b4b#diff-f2d6a4d46b2aeaccd8ce40e4d3ea753dR68. I fail to see why this should be an issue? Can someone from node-sass team (@am11) test if reversing the order for subset_map and _skip_source_map_update fixes the issue?

[am11]

Yes I will take a look now. :)

Actually that test was removed recently (sass/node-sass#492 (comment)). I will put it back in our repo and get back to you.

[mgreter]

I'm pretty confident that this will solve the issue. I have already commited the change to master as it at least gets rid of the clang warning.

[xzyfer]

Thanks for that @mgreter

[am11]

👍

But I couldn't test it with windows due to this: #591 (comment) 😢

[xzyfer]

@am11 are you able to compile master on Windows now? @mgreter has pushed the _skip_source_map_update patch directly to master in 2fae47a

[am11]

Let me give it a try. (-8

[am11]

It compiled without any issue! 😄 👍
But that test is still failing. 😞 👎

[xzyfer]

Thanks for taking a look. I've reopened #586 to track this issue.