-
Notifications
You must be signed in to change notification settings - Fork 280
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
Use stored source instead of Unparser to format ASTs #827
Conversation
It seems the thing to do is to implement a simple formatter in each of our SexpExtensions, for the relevant node types. I wouldn't use |
Yes, exactly. We'd need to drop SexpFormatter completely, first of all because we'd remove |
fdbacea
to
66be5f2
Compare
unparser
.
@troessner, @chastell this is ready for review. I went a completely different route, using the actual source of the expression as provided by parser. |
@@ -18,7 +18,7 @@ Feature: Offer different ways how to load configuration | |||
""" | |||
smelly.rb -- 3 warnings: | |||
[4, 5]:DuplicateMethodCall: Smelly#m calls @foo.bar 2 times [https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md] | |||
[4, 5]:DuplicateMethodCall: Smelly#m calls puts(@foo.bar) 2 times [https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md] | |||
[4, 5]:DuplicateMethodCall: Smelly#m calls puts @foo.bar 2 times [https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not leave the brackets? I find this better readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now using the actual source text so it depends on how it's actually written there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense ;)
First review -> done. |
AST::Node stores the parsed source so we can use that directly. As a result, some smell masking may have to be adjusted. Since SexpFormatter is no longer needed, it is removed, as is its dependency, Unparser. Since Parser 2.3.0 behaves differently on certain syntax errors and support for it is part of an upcomping change, the allowed versions of Parser are limited, as Unparser did in the past.
Great job, merging! |
Use stored source instead of Unparser to format ASTs
No description provided.