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

Add model validation in method modeling panel #2936

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

koesie10
Copy link
Member

This adds validation of the modeled methods to the method modeling panel. It implements the following two validations:

  1. A neutral classification cannot be combined with any other non-neutral classification
  2. A classification cannot be duplicated

Multiple neutral classifications will be validated by the second validation rather than the first one.

Screenshot 2023-10-10 at 14 32 01

Screenshot 2023-10-10 at 14 32 12

Testing is only really possible in Storybook right now, without any interactivity.

Checklist

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

@koesie10 koesie10 marked this pull request as ready for review October 10, 2023 13:17
@koesie10 koesie10 requested review from a team as code owners October 10, 2023 13:17
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Looks reasonable, with a couple of questions.

We'll likely end up displaying this info in the model editor and method modeling panels. Do we want to perform the validation once in each location, or do it in a shared location and pass the errors into each view?

I think what you have here where we do the validation in the view is likely fine. It will mean we do the validation twice, but the validation should be fast enough and consistent, so doing it twice is safe. At least this way the validation errors will definitely match the data being displayed.

result.push({
title: "Duplicated classification",
message:
"This method has two identical or conflicting classifications.",
Copy link
Contributor

Choose a reason for hiding this comment

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

In the extreme case of more than two duplicate modelings, do we want to avoid adding multiple errors? I'm unsure. I'd say it's a very unlikely case, so perhaps not worth putting in the implementation effort to give a nice user experience in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure either, but I don't think it's worth handling this case for a very rare case.

extensions/ql-vscode/src/model-editor/shared/validation.ts Outdated Show resolved Hide resolved
@koesie10
Copy link
Member Author

We'll likely end up displaying this info in the model editor and method modeling panels. Do we want to perform the validation once in each location, or do it in a shared location and pass the errors into each view?

I think what you have here where we do the validation in the view is likely fine. It will mean we do the validation twice, but the validation should be fast enough and consistent, so doing it twice is safe. At least this way the validation errors will definitely match the data being displayed.

I think this is a lot easier to manage. If we were to store the validation errors in the modeling store, we would need to run the validation for every method, even if it's not shown in the model editor (for example if the group it's in is collapsed). The validation should always result in the same result, so keeping it in the view shouldn't really make a big difference. If we start experiencing performance problems because of this, we can always consider moving it to the extension host where the user will probably notice it less.

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

👍🏼

@koesie10 koesie10 merged commit 7e3cb75 into main Oct 12, 2023
14 checks passed
@koesie10 koesie10 deleted the koesie10/method-modeling-panel-validation branch October 12, 2023 11:08
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