Skip to content

Wire up processing model evaluation run results and showing alerts#3503

Merged
charisk merged 2 commits intomainfrom
charisk/model-alerts
Mar 22, 2024
Merged

Wire up processing model evaluation run results and showing alerts#3503
charisk merged 2 commits intomainfrom
charisk/model-alerts

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Mar 21, 2024

Wires up the model evaluator to trigger loading of variant analysis repository results, process them and update the model alerts view.

In the absence of alert provenance information, we fake process the analysis results for now, and return mock results.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested review from a team as code owners March 21, 2024 14:50
@charisk charisk force-pushed the charisk/model-alerts branch from cb44355 to dcc34a8 Compare March 21, 2024 15:20
@charisk charisk force-pushed the charisk/model-alerts branch from dcc34a8 to fd808c8 Compare March 21, 2024 15:36
Comment on lines +757 to +758
| SetRepoResultsMessage
| SetReposResultsMessage;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the difference between the SetRepoResultsMessage and SetReposResultsMessage is not clear from the name. The data seems to be exactly the same, so it's quite confusing (at least to me) when we have both messages and they have different functions. Perhaps a name like ReplaceRepoResultsMessage would be better (based on its function in the view code)? The same might be useful for the functions on the model alerts view, something like updateRepoResult and replaceRepoResults?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree it's quite confusing. I think that setReposResultsMessage is the correct name for what I'm adding, but that the other messages (and related view functions) should be "addRepoResults" or something like that. Essentially set should be used when we're actually setting the whole thing, otherwise we should use some other term.

I was hoping to tidy this up in a future PR, what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me, I don't think this needs to be blocking for this PR.

Comment thread extensions/ql-vscode/src/model-editor/model-evaluator.ts Outdated
Comment thread extensions/ql-vscode/src/model-editor/model-evaluator.ts
Comment thread extensions/ql-vscode/src/model-editor/model-evaluator.ts Outdated
Comment thread extensions/ql-vscode/src/view/model-alerts/ModelAlerts.tsx Outdated
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@charisk charisk merged commit cbc6b73 into main Mar 22, 2024
@charisk charisk deleted the charisk/model-alerts branch March 22, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants