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

Fixes regression with custom precision (#364) #573

Merged
merged 1 commit into from
Oct 28, 2014

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Oct 27, 2014

I do not know what the original commit that introduced this regression is doing. This approach is pretty hackish, but IMO not much more than the previous "fix". I've added another bool to context to let string eval have it its own way. Not really pretty but works!

I guess this is better than nothing and if I did break the other fix, it will probably come up soon again :)
If other committers agree, this can be merged. If there are no objections, I will merge it in soon!

@mgreter
Copy link
Contributor Author

mgreter commented Oct 28, 2014

Maybe someone could review it? Would really like get this off the list!

// CC @xzyfer, @am11

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2014

I agree it's rather hacky. It'd be great to get some context on what the original breakage was. Ideally we could more this fix into inspect.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2014

In the interest of increasing ruby sass compatibility I'm ok with merging this as long we open another issue to come back around to cleaning this up.

@mgreter
Copy link
Contributor Author

mgreter commented Oct 28, 2014

ACK! I guess issue #324 may also be related? IMO source maps are not yet working perfectly, but unfortunately not many people (including me) seem to understand what the code really does ...

@am11
Copy link
Contributor

am11 commented Oct 28, 2014

This looks fine. The old "fix" itself was a hack. This one at least doesn't break the existing feature.

On a slightly separate note, do you guys have any idea how to make the sourcemap capture more details than it already does? Source map is the only thing that I wish libsass take LESS lead (which has tons of sourcemap options) rather than the ruby-sass, which itself needs better source-maps: #324. The idea really is to make "every token" in generated output mapable to the original code. "{", "}", ":", "color", "violet" etc. are the tokens. Do you guys think we can pull that one off? I tried but couldn't figure out the internals of parser, ast et el. I'm still a rookie when it comes to dark arts of libsass internals. 🐰

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2014

My concern with this (and previous) implementation is that there's no context as to what's happening. This reflects the lack of clear understanding of the problem. We're building a house of cards here which worries me.

I'm pretty confident on the parser's internals, but a bit vague on the sourcemaps integration. IMHO there's going to dedicated time for dealing with sourcemaps and a good test suite. It would be worth finding an advocate for sourcemaps to help us build that required test bed.

HamptonMakes added a commit that referenced this pull request Oct 28, 2014
Fixes regression with custom precision (#364)
@HamptonMakes HamptonMakes merged commit ca5f271 into sass:master Oct 28, 2014
@mgreter mgreter deleted the fix/custom-precision branch April 6, 2015 17:14
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.

4 participants