Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toolkit Annotation support #186

Open
bryanmacfarlane opened this issue Oct 10, 2019 · 15 comments
Open

Toolkit Annotation support #186

bryanmacfarlane opened this issue Oct 10, 2019 · 15 comments
Assignees
Labels
core enhancement New feature or request

Comments

@bryanmacfarlane
Copy link
Member

The toolkit should support first class support for adding annotations

export function addAnnotation(annotation: Annotation): void 

Annotation object will match this

Annotation level will be a toolkit enum which gets written as a string to the annotation. We need to add Notice to the timeline issue

export enum AnnotationLevel {
  Notice = 0,
  Warning = 1,
  Failure=2
}

the equivalent cmd will accept properties that match the annotation properties

echo ::add-annotation annotation_level=error, path=css/main.css, start_line=0, end_line=0::myMessage

toolkit error and warning will emit [error]message and [warning]message which the UI will decorate red or yellow. (error and warning does not currently support an annotation). The web UI should colorize but string the [error] and [warning] from the view (but still in the logs)

The existing logging error and warning command will emit [error]message and [warning]message. Even though not documented, they take properties (problem matchers). If those properties are used, then for compat, create an issue object which will become an annotation service side.

After this is complete, change problem matchers to use the add-annotation cmd or toolkit methods.

@TingluoHuang
Copy link
Member

TingluoHuang commented Oct 10, 2019

Today the problem matcher produce:

{
  "line": 1,
  "column": 2,
  "severity": 3,
  "message": 4,
  "code": 5,
}

@bryanmacfarlane
Copy link
Member Author

^^ yep, and per note from description above, if the error or warning cmd come across with those properties, it will (for compat) make it's way to an issue object and then an annotation. problem matchers will be changed to write the new command syntax. Also above is not doc'd for error/warning and will not be.

@bryanmacfarlane
Copy link
Member Author

@thboop is picking up this work today.

@dixonwille
Copy link

Maybe I am miss understanding the "no documentation" around error and warning. I notice in this repository it is not but here it is. I have an action using the error command and expected an annotation (docs don't say it explicitly, but that was the only benefit I could see with adding the info to the message). I ended up here realizing it isn't supported yet.

Would really love this feature!

@svartalf
Copy link

Adding to @dixonwille message, it would be nice to clarify if it is okay to output multiline messages with ::error/::warning

@bryanmacfarlane
Copy link
Member Author

@dixonwille - yes, apologies. we intended to hold back on the documentation as it was going to be removed. We are preparing a patch to get back inline with the docs and hopefully go out tomorrow. we will probably abandon this specific issue and redefine toolkit error, warning with new optional parameters (as opposed to a new method) so it aligns with the documented command.

@svartalf, can you log a new issue with the question on multiline? I will follow up.

@garyttierney
Copy link

On this point:

After this is complete, change problem matchers to use the add-annotation cmd or toolkit methods.

Are problem matchers supposed to produce annotations at the moment (via some other mechanism)? I did try to port over one I had from vscode to no success,

@joshmgross
Copy link
Member

Are problem matchers supposed to produce annotations at the moment (via some other mechanism)?

@garyttierney Yep! Problem matchers will produce annotations, you can check out some of the actions/setup- repositories to see how they have their problem matchers set up.

Example from setup-go: https://github.com/actions/setup-go/blob/master/.github/go.json

Example Run with a Go syntax error

Let me know if you need any help setting that up! It's possible your regex isn't catching your errors or your problem matcher isn't registered (use the command ex: ::add-matcher::path/to/matcher.json)

@TrueBrain
Copy link

TrueBrain commented Oct 29, 2019

Sorry to hijack this issue a bit, but to follow up on your comment @joshmgross:

Setting severity in the ProblemMatcher seems to have no effect. Like:
"problemMatcher": [ { "owner": "test", "severity": "warning", ...
Is this not yet implemented, or I am doing it wrong? (I got the idea from https://code.visualstudio.com/docs/editor/tasks-appendix, under interface ProblemMatcher)

Currently I work around this by a slightly ugly sed, that allows severity in the pattern.
https://github.com/TrueBrain/actions-flake8/blob/initial_work/entrypoint.sh#L13

@joshmgross
Copy link
Member

joshmgross commented Nov 6, 2019

@TrueBrain

Is this not yet implemented

I asked @thboop about this, no currently we don't support a severity for problem matchers. We have a similar syntax to VS Code, but it's not a full implementation. Feel free to file an issue, I think that would be a great feature to add 😄

@thboop
Copy link
Collaborator

thboop commented Nov 6, 2019

Sorry to hijack this issue a bit, but to follow up on your comment @joshmgross:

Setting severity in the ProblemMatcher seems to have no effect. Like:
"problemMatcher": [ { "owner": "test", "severity": "warning", ...
Is this not yet implemented, or I am doing it wrong? (I got the idea from https://code.visualstudio.com/docs/editor/tasks-appendix, under interface ProblemMatcher)

Currently I work around this by a slightly ugly sed, that allows severity in the pattern.
https://github.com/TrueBrain/actions-flake8/blob/initial_work/entrypoint.sh#L13

We do support group numbers for severity, which expects a group number containing either 'warning' or 'error' case-insensitive. However, you can't pass in 'error' to the problem matcher directly.

If you would like that feature, please create another issue with an enhancement request!

This pr adds Problem Matcher documentation for your reference: #198

@polothy
Copy link

polothy commented Feb 16, 2020

Hello! Is there an update in the direction for this? Are one of the methods going to gain feature parity with the GitHub Annotations API?

Looking to try this out and I need to convert a linter to something for annotations. Leaning towards converting the linter output into a custom format and then making a matcher for it. Would be nice though if I didn't have to do the two step process and something like this just supported all the GitHub Annotation API properties.

Either way... really like this concept of being able to add annotations without needing an API token and doing API calls 👍 💯

@TDHolmes
Copy link

Any update on where this is going?

@thboop thboop added the core label Apr 29, 2021
@freaktechnik
Copy link

freaktechnik commented Aug 23, 2021

It seems the logging functions now support some annotations, but not path, so you can't actually attach them to a file...

@luketomlinson
Copy link
Contributor

Good catch @freaktechnik. I filed an issue: #892, which hopefully we can get to soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

No branches or pull requests