Feature js in smell #312
Feature js in smell #312
Conversation
|
@julian-berbel this is just a draft but if you give me your feedback I can finish it without disturbing |
|
|
||
| def ast(content, **args) | ||
| @tool.call(content) rescue nil | ||
| if args[:serialization] |
There was a problem hiding this comment.
I'm not sure I get this bit. If serialization is set to true, tool won't be called? what does that mean for extensions like mulang-ruby which use this for parsing?
There was a problem hiding this comment.
Perhaps it is a bit tricky, but yes, it will be called: super calls ast_analysis, which calls base_analysis, which finally calls sample, which polymorphically in this class class call_tool.
I think it is ok but just because I am changing implementation semantics here, so that the main source of truth of the ast generation is in call_tool, not in ast anymore. As a consequence, both methods that require calling the ast generation tool - sample and ast - delegate on it.
There was a problem hiding this comment.
However I am not a fan of this implementation, how would you implement it?
There was a problem hiding this comment.
PS: this was not actually part of this original PR, I should have send as a separate one 🤣
There was a problem hiding this comment.
I see!
However I am not a fan of this implementation, how would you implement it?
If I'm understanding correctly you could just do super in either case (not overriding the method) then, right? but simply calling tool on second case is faster? I'm fine with that
There was a problem hiding this comment.
Yes, exactly. The original behavior was to always simply call tool. However, this is not good we when actually want serialization - or maybe some other future flags, like e.g. force_normalization, so I added this case.
There was a problem hiding this comment.
PS: I have merged this as a separate PR #314
flbulgarelli
force-pushed
the
feature-js-in-smell
branch
2 times, most recently
from
44624ec to
04af03c
Compare
flbulgarelli
force-pushed
the
feature-js-in-smell
branch
from
04af03c to
f9ff61a
Compare
No description provided.