Closed Cisplatin closed 6 years ago
@Cisplatin this is MUCH cleaner(I'd like to hear you if you have any opinion on below points)
We will provide these methods to rule devs and even encourage them. But there could also be a downside - devs might get confused because they end up checking using type
of some nodes and for other nodes they use util functions. Another type of confusion could be ambiguity (for eg - "how do I check for an else
clause? is there as isElse
util or should I use isIf
only?" Because checking docs is simply more time consuming, they might resort to checking using type
). Idk how it turns out, so I think we should introduce these as experimental.
I can see some major benefits of having utils. 1 especially is that I'm planning to move solium to antlr4 based parser (the current solidity parser is the bottleneck and makes solium very slow - the biggest pain point). The new parser might have different type
s and so these utils can abstract away all those details.
I think all these utils are better suited to solium (core repo), which in turn, can expose them to both core rules and plugin rules. Otherwise everyone will end up writing one for their own plugins
@duaraghav8 Yeah, the first point is why I asked you about this PR on discord - devs might just resort to checking type
, like you said, in which case this PR will probably do more harm than good. I agree this should be an experimental PR.
With regards to putting them in the core solium repo, that's also a good point - would you like me to move the utils
file to there and then change this PR to use that file instead?
@Cisplatin Yes, that's the best way to move forward! Please move the utils to solium, expose them & use in this plugin.
ast-utils.js
- https://github.com/duaraghav8/Solium/blob/master/lib/utils/ast-utils.jsINHERITABLE_METHODS
IN https://github.com/duaraghav8/Solium/blob/master/lib/utils/source-code-utils.jsfeel free to use ES6 features at ALL the places
This should expose all util methods to the rules like
create(context) {
if (context.getSourceCode().isExpression(node)) {
// blah
}
}
Tests for above go into https://github.com/duaraghav8/Solium/blob/master/test/lib/utils/ast-utils.js and https://github.com/duaraghav8/Solium/blob/master/test/lib/utils/source-code-utils.js (and any other relevant files)
If you feel there's a more intuitive/cleaner way to do this, I'd like to hear it
@duaraghav8 Sounds like a clean solution - I'll get working on it!
@duaraghav8 Going to close this PR and make a new one that uses Solium's utils to clean-up the rules since this PR has quite a few conflicts now due to refactors and such.
Some of them (the utils) might not be too useful, I'm not sure - let me know if this is a good idea or not!