OpenEdge plugin - Request to include in Marketplace

Plugin Development
#1

Hello,

I would like to include the OpenEdge plugin in the SonarQube marketplace. OpenEdge is a quite old (but still alive !) platform from Progress, mostly for business applications (ERP systems, banking, insurance, industry, …). The plugin includes a parser for the language, various metrics, copy-paste detection, import of compiler warnings as issues, a rules engine and a few basic rules. Custom rules can be written, as the plugin gives full access to the AST and parsing information ; a sample project is available here. Additional rules are also commercially available.

Thanks !

Gilles

#2

Hi Gilles,

As you’re probably aware, one of the requirements for Marketplace inclusion is that the plugin be tested by SonarSource staff. 99.9% of the time that means me, and I’m about to be done for the year. I’m setting this aside for January, but it may even be mid-month before I can come back to you.

 
Ann

#3

No problem Ann, enjoy your holidays :christmas_tree: ! I’ll post a reminder mid-January in case you didn’t follow up :slight_smile:

Gilles

1 Like
#4

Hi,

I had a few minutes free & took a quick look at this. For the bureaucratic requirements, this mostly looks good. Unfortunately, you’ve only ever analyzed once on SonarCloud so no Quality Gate is computed. If it were, I suspect you would fail; there’s a D Reliability rating and coverage is at 67.7%. I suspect a point release would be sufficient for those things. :wink:

Beyond that, can you point me to a project with all the necessary files in place so all I have to do is run analysis & look at the result?

 
Thx,
Ann

#5

Ann,

The project was previously analyzed here: https://sonar.riverside-software.fr/dashboard?branch=develop&id=eu.rssw%3Asonar-openedge
Quality gate was green, as almost green ( :grimacing: ) on SonarCloud for the develop branch. I’ll fix the remaining issues !

This project contains the necessary files to run the analysis and look at the results: https://github.com/Riverside-Software/sonaroe-test-full
No need for the OpenEdge proprietary compiler, I’ve included the full build output.

Gilles

#6

Hi Gilles,

Thanks for repeating the link to the demo project. I see now that you included it in your original post. Generally this looks good. There are some things that IMO should be tweaked.

Must do
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 :smiley:)

Nice to have

  • Your setting descriptions could be a little clearer to me. For instance:

    Backslash as escape char
    Does backslash escape next character on Windows ?

    I’m on Linux. If I turn this on… nothing happens? That’s what I take from reading the description, but I’m skeptical that’s actually the case. Either way a little more text clarifying it would be good.

  • 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? :smiley:):

    Comma-separated list of suffixes of OpenEdge files to analyze.
    Default value: p,w,t

    For comparison, in the built-in languages the default values actually appear in the input:

    Selection_999(368)
    Selection_999(368)800×155 18.2 KB

    Making it work like this would be even better!

  • 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.

  • 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.

For all that, this is a remarkably clean first request. Kudos on that!

 
Ann

#7

Hi Ann,

Thank you for taking time to review the plugin. Comments inline.

Must do
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 :smiley:)

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 :slight_smile:

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? :smiley:):

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.

Gilles

#8

Hi Gilles,

Yes, I saw that setting. That’s how I figured out the plugin phones home. Not everyone is going to comb through the settings. That’s why I’m asking that you make a note in the docs as well.

And speaking of docs, thanks for the pointer to them. In my experience, most folks just use the README, which is why I was looking there. Maybe as a kindness to your users also add a pointer from the README to the wiki? (Totally up to you.)

Once I got to the Wiki I saw something I had missed in my previous look at your plugin: you’ve added a “licenses” entry under Administration -> Configuration. From what I can tell in-app and from your documentation, it relates to a second, separate, commercial plugin? Based on the documentation, it seems to be a duplication of the licensing that’s already present in your commercial plugin. Perhaps it’s included in this plugin by mistake?

To be candid, I’m not comfortable with this setting, and I’m adding sorting this out to the “Must do” list.

I suspected that might be the case. Since your users will (presumably) know the language this is as I said just a nice-to-have :slightly_smiling_face:

Yes! FWIW, a shorter version (which I believe makes clear noun-vs-verb) is “File suffixes for includes”. And starting with “File suffixes” will have the knock-on benefit of putting it next to the “File suffixes” setting in the page.

:smiling_face_with_three_hearts:

 
Ann

#9

Hi Ann

Yes, I saw that setting. That’s how I figured out the plugin phones home. Not everyone is going to comb through the settings. That’s why I’m asking that you make a note in the docs as well.
And speaking of docs, thanks for the pointer to them. In my experience, most folks just use the README, which is why I was looking there. Maybe as a kindness to your users also add a pointer from the README to the wiki? (Totally up to you.)

Assuming the entrypoint is the GitHub main page, this page now clearly points to the Wiki pages. And from those Wiki pages, the user is directed to the Getting Started page where I’ve added an entry for the analytics settings in the sample set of properties. As there’s no such thing as Maven or Gradle in OpenEdge, users have to manually configure all properties, and it’s almost sure that they will start by copy-pasting the sample file. Is that enough ?

Once I got to the Wiki I saw something I had missed in my previous look at your plugin: you’ve added a “licenses” entry under Administration -> Configuration. From what I can tell in-app and from your documentation, it relates to a second, separate, commercial plugin? Based on the documentation, it seems to be a duplication of the licensing that’s already present in your commercial plugin. Perhaps it’s included in this plugin by mistake?
To be candid, I’m not comfortable with this setting, and I’m adding sorting this out to the “Must do” list.

You’re absolutely right, this menu entry is only required for the commercial set of rules. I’ll move that away from the open-source plugin, and only include it in the other plugin. I’ll update the pull request for the Sonar Update Center as soon as it’s ready.

Your other comments about the property descriptions already led to changes, that will be included in the next release !

Gilles

#10

Hello Ann,

Pull Request has been updated in order to use the latest release which doesn’t inject the menu entry.
Let me know if you see anything else !

Gilles

#11

Pull request has been updated. Is there anything else I’d have to add ?

Gilles

#12

Hi,

This is on my side & I just haven’t had time to deal with it. Sorry for the delay. I’ll come back to this as soon as I can.

 
Ann

#13

Hi,

FYI, not forgotten. This open tab taunts me every day.

 
Ann

#14

Did you close your web browser ? :wink:

Gilles

#15

Tabs persist across sessions.

And yes, this is frightfully, embarrassingly tardy. :flushed:

#16

Hi,

Deeply sorry it has taken me so long to come back to this.

I’ve just loaded the updated version of the plugin & I see the license flub has been taken care of. :+1:

Unfortunately, I’m not satisfied with the treatment of the “phone home” setting. You’ve added an explicit mention as the last value in a sample analysis parameters document one or two clicks (depending on the path) from the project homepage. However,

  • server-side settings are overridden by analysis parameters on a per-analysis basis, not permanently
  • the fact that you’ve exposed this as an analysis parameter and (I’m just realizing) a project-level setting implies that the plugin will phone home once per analysis…?

This really needs to be documented as a separate entry, and not just slipped in at the end of a very long list.

 
Ann

#17

I’ll see what can be done with this setting ; in fact, I think it can be removed as it’s now not as useful as it was at the very beginning of the project. I’ll take care of this point next week.

#18

Ann,

The analytics property has been moved to the additional set of rules, so the main plugin doesn’t send any data. The version with this change has just been released ; and the pull request for sonar-update-center has been updated.

Gilles

#19

Hi Gilles,

You have a failing Quality Gate on SonarCloud…?

If it weren’t for that, you’d be in now. How about a 2.13.1?

 
Ann

#20

My bad… :man_facepalming:
Will fix that.