Thank you for taking time to review the plugin. Comments inline.
From looking at the language settings, it appears that your plugin phones home. Please add a mention of that to your docs. (BTW, I’m taking your project README as your docs. If you have docs somewhere else then a pointer would be good )
By “phones home”, is that the ping to our analytics server ? If so, the
sonar.oe.analytics is present in the list of analysis parameter. It sends metrics only for the OpenEdge language (lines of code and rules execution time), and can be disabled by setting the property to false.
Documentation is something that has to be improved ; the wiki is where most of the documentation can be found.
Nice to have
- Your setting descriptions could be a little clearer to me.
The descriptions are clear when you know the language, but I can see how obscure they can be without knowing it. I’ll include a short explanation here, and will update the documentation later.
Backslash as escape char
Does backslash escape next character on Windows ?
Compiling OpenEdge code on Windows and on Linux can generate a different result, because the escape character on Linux can be a backslash or a tilde, but only a tilde on Windows (the backslash doesn’t escape anything). That sounds stupid (and probably is), as some code can be compiled on Windows but not on Linux because of misplaced backslashes. And yes, there’s a rule to tell if a backlash is not properly escaped, so that OpenEdge code can be compiled on any OS
File suffixes and Include file suffixes - you give what I’m guessing are actually the defaults (since my analysis worked) as “e.g.”. It would be more helpful if you explicitly listed the defaults as such. E.G. (see what I did there? ):
There are no strict rules for file extensions in OpenEdge, though the recommendation always has been .p for procedures, .cls for classes, .w for GUI elements and .i for include files. The difference between file suffixes and include file suffixes is that the full parser and rules are executed are executed on standard files, while include files have only the code highlighter being executed. I’ll check why the default values are not being displayed, that’s surely a bug in our property definition.
In my literal-minded opinion you should rename the “Include file suffixes” setting. My first 2 readings of this made me think it was about inclusions/exclusions, rather than about recognizing source files.
Ah ! From my OpenEdge background, I read that as ‘include file’ suffixes, and not with include as a verb as any English reader would do. I’ll try to find a better description for that, but it may end up as “File suffixes for include files”. Is that better ?
The other settings could benefit from fuller descriptions. For instance, for “Skip rcode parsing” the description is… “Skip rcode parsing”. Maybe instead give me a hint of why I do/don’t want to do this parsing? Maybe give me a hint why I want to filter byte ranges from XML files? And so on.
Definitely agree on that, description will be improved !
For all that, this is a remarkably clean first request. Kudos on that!
Thanks ! On my side, I’d like to congratulate the SonarQube team for the quality of the API, and making it possible to extend the platform and support new languages ! It’s been a long journey to develop this plugin, and I hope it will be successful for a long time.