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

Remove To_String by using AST->to_string and inspect #1858

Merged
merged 2 commits into from
Jan 17, 2016

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Jan 16, 2016

This cleans up the usage of perform(&to_string) by only using inspect. Also adding additional TO_SASS output mode, since ruby sass has differences between to_sass and inspect and both are used in various places (i.e. error reporting). Now every AST_Node has a to_string, to_sass and inspect method, and all return a std::string. They all internaly use the inspect visitors, so we only need to implement all types only once for all variations in one place. With this I hope we can more easily match ruby sass and I already saw one or two things that need to be implemented for certain options. The to_sass mode specially is responsible to add parentheses were needed.

This is basically the continuation of #1754

@mgreter mgreter force-pushed the feature/cleanup-to-string-visitor branch from 87d0333 to fc75b8f Compare January 16, 2016 23:37
@mgreter mgreter force-pushed the feature/cleanup-to-string-visitor branch from fc75b8f to c17f656 Compare January 17, 2016 00:01
@mgreter mgreter force-pushed the feature/cleanup-to-string-visitor branch from c17f656 to adf05c4 Compare January 17, 2016 00:45
@mgreter mgreter added this to the 3.3.3 milestone Jan 17, 2016
@mgreter mgreter self-assigned this Jan 17, 2016
@mgreter
Copy link
Contributor Author

mgreter commented Jan 17, 2016

@xzyfer I'm thinking about renaming Inspect visitor class to Stringify, because the name is IMO a bit confusing, since it handles all the stringification now. In terms of ruby sass it never did what ruby sass internaly does with inspect. We should use to_string, to_sass and inspect methods on AST_Node instances. These correspond to ruby sass perform, #{node.to_sass(opts)} and #{node.inspect}.

I also want to add an emitter that does not create a sourcemap, since for these methods it's not necesarry (but that's really just a performance optimization). Things start to come together.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 17, 2016

@mgreter Ruby Sass calls it the ToCss visitor. Seems like an ideal name to me?

@xzyfer
Copy link
Contributor

xzyfer commented Jan 17, 2016

Now every AST_Node has a to_string, to_sass and inspect method, and all return a std::string.

This is a great change. I wasn't expecting to implement this until 3.4. Nice work.

mgreter added a commit that referenced this pull request Jan 17, 2016
Remove `To_String` by using `AST->to_string` and `inspect`
@mgreter mgreter merged commit 0f95e38 into sass:master Jan 17, 2016
@saper
Copy link
Member

saper commented Feb 14, 2016

I am getting this now:

gmake BUILD=debug-shared CC=cc CXX=c++
mkdir lib
cc -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/cencode.o src/cencode.c
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/ast.o src/ast.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/base64vlq.o src/base64vlq.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/bind.o src/bind.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/color_maps.o src/color_maps.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/constants.o src/constants.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/context.o src/context.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/cssize.o src/cssize.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/emitter.o src/emitter.cpp
c++ -g -DDEBUG -DDEBUG_LVL="NONE" -Wall -DLIBSASS_VERSION=""3.3.3-19-g3054"" -std=c++0x -I /home/saper/sw/libsass/include -fPIC -fPIC -c -o src/environment.o src/environment.cpp
src/environment.cpp:183:38: error: no matching member function for call to 'to_string'
{ std::cerr << " : " << val->to_string(true, 5); }
~~~~~^~~~~~~~~
src/ast.hpp:94:25: note: candidate function not viable: requires single argument 'opt', but 2 arguments were
provided
virtual std::string to_string(Sass_Inspect_Options opt) const;
^
src/ast.hpp:95:25: note: candidate function not viable: requires 0 arguments, but 2 were provided
virtual std::string to_string() const;
^
1 error generated.

is it because of this change?

@xzyfer
Copy link
Contributor

xzyfer commented Mar 11, 2016

@saper is that error still happening on b6841ed? I'm unable to reproduce.

@saper
Copy link
Member

saper commented Mar 14, 2016

Yes, see #1917 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants