Page MenuHomePhabricator

Harbormaster linter report for UI and API have some confusing names
Closed, ResolvedPublic

Description

the field titled "Message" in the UI is titled "name" in the API, and confusing with the separate "description" field in the API.

Event Timeline

Please follow all guidelines in Contributing Bug Reports. These are required on all bug reports.

chad renamed this task from Harbormaster linter report for buildable renders linter name in description column to Harbormaster linter report for buildable renders linter name in message column.Dec 15 2016, 5:17 PM

It's currently unclear to me if this is a bug report or feature request. The code appears to be working as intended, and adding the description to the property table would be an added feature.

I'll try to give an example which makes clear that linter message is not meant to be linter name.

A simple .arclint:

{
        "linters": {
                "testlinter": {
                        "type": "script-and-regex",
                        "include": "(testlinter.sh)",
                        "script-and-regex.script": "./testlinter.sh",
                        "script-and-regex.regex": "/^(?P<name>lintername):(?P<code>CODE):(?P<severity>advice):(?P<message>message)/m"
                }
        }
}

Note the name ?P<message> which becomes description in the next step.

Linter script:

#!/bin/sh

echo "lintername:CODE:advice:message"
exit 0

Output of arc lint --everything:

>>> Lint for testlinter.sh:


   Advice  (CODE) lintername
    message
 OKAY  No lint warnings.

Message is the displayed message with details (as intended).

This is the same as JSON:

{
    "testlinter.sh": [
        {
            "line": null,
            "char": null,
            "code": "CODE",
            "severity": "advice",
            "name": "lintername",
            "description": "message",
            "original": null,
            "replacement": null,
            "granularity": 1,
            "locations": [

            ],
            "bypassChangedLineFiltering": null,
            "context": "#!/bin/sh\n\necho \"lintername:CODE:advice:message\"\nexit 0\n"
        }
    ]
}

Since Harbormaster does not understand this JSON format, it needs to be converted into the Harbormaster linter result JSON format. According to docs, it looks like this:

{
        "buildTargetPHID":"PHID-HMBT-xxx",
        "type":"pass",
        "lint":
        [
            {
                "name": "lintername",
                "code": "CODE",
                "severity": "advice",
                "path": "testlinter.sh",
                "description": "message"
            }
        ]
}

After piping this to arc call-conduit harbormaster.sendmessage, you get this result in the linter box:

linter-view-in-harbormaster.png (108×700 px, 8 KB)

What was message before, becomes linter name in Message column. This is confusing, in my opinion.

I've just noticed that arc lint --everything output summary gives me this:

testlinter.sh::Advice: lintername
 OKAY  No lint warnings.

This, of course, made me wonder what the documentation says.

It seems that just the title of the table and/or the script-and-regex linter matcher ?P<message> have confusingly similar names. Also the name field in the JSON files does not refer to a linter name but the name for the test. It would be less confusing to use less generic name for the identifier that makes clear that this is a test name or test title.

On the other hand, it is not possible to tell which linter a message originates from (really linter name), but this is another story.

What made the translation from the string-after-regex to the harbormaster json structure in your example?

It's my own script. I haven't found anything like this in the sources. I also haven't found any hints how to integrate`arc lint` without pain (no --target option like for arc unit).

OK, looks like this is just a case of miss-reading the docs:

  • name (optional) Text summarizing the lint message. For example, "Syntax Error".

name in the regex does not mean "Name of linter" but "Title of error".

That's also what it means in harbormaster.sendmessage, which is the modern json format for lints.

There is actually no field in the lint json that means "Name of linter", in any of the formats.

avivey claimed this task.

Partially it is. I remember now, I've fallen in this trap the second time now, I forgot what name is (again, btw). I told you that name is too generic. The first time I found out that Message is a wrong title for Linter rule name and worked around it. This time I forgot it again, because the terminology is not consistent.

Still, in Harbormaster it's not the Message that is displayed in the column but the Rule/test name or something like that. A "linter message" is not an adequate row title, because it is rather associated with the "description".

avivey renamed this task from Harbormaster linter report for buildable renders linter name in message column to Harbormaster linter report for UI and API have some confusing names.Dec 16 2016, 9:12 PM
avivey updated the task description. (Show Details)

There may be some merit to changing the UI to say "Title" or "Error Title" instead of "Message", but this is so low in terms of value ("Admins that don't read docs confuse fields") that I doubt this will actually happen.
Changing the API is even less likely, because that will actually break other people's code.

The API is considered unstable and there seems to be work to do about this manual transformation from arc lint --output json (which isn't even JSON) to harbormaster message JSON format.

But I understand of course that changes to APIs, no matter how small, are extremely annoying.

I've been also playing with the shellcheck linter and evaluating it using the following .arclint entry:

		"shell-scripts": {
			"type": "script-and-regex",
			"include": "(\\.sh$)",
			"script-and-regex.script": "sh -c 'shellcheck -f gcc \"$0\" || true'",
			"script-and-regex.regex":
			"/^(?P<file>[^:]*):(?P<line>[0-9]*):(?P<char>[0-9]*):((?P<advice>note)|(?P<severity>[^:]*)): (?P<message>)(?P<name>.*) \\[(?P<code>[A-Z]*[0-9]*)\\]$/m"
		}

As you can see I need to match the actual linter message with ?P<name> and workaround the fact that there is no linter message with ?P<message> and an empty match. Otherwise I get the boring entry "Lint" in the column Message in Harbormaster.

This is not very optimal at the moment.