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

Implement editing / saving multiple modeled methods from the model editor #2952

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

robertbrignull
Copy link
Contributor

Switches to using SetMultipleModeledMethodsMessage in FromModelEditorMessage, and therefore allows editing and saving multiple modeled methods in the model editor.

The only tricky bit was constructing the onChange handlers for the input components. That bit could use some scrutiny when reviewing.

Tested locally and appears to be working correctly, both when the feature flag is disabled and enabled.

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.

@robertbrignull robertbrignull requested review from a team as code owners October 11, 2023 12:47
Comment on lines 110 to 112
: viewState.showMultipleModels
? modeledMethodsProp
: modeledMethodsProp.slice(0, 1),
Copy link
Member

Choose a reason for hiding this comment

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

Nested ternaries are quite hard to read. Could you extract this somehow, for example by lifting the modeledMethodsProp.length === 0 check out of this ternary and moving it before the ternary?

Copy link
Contributor Author

@robertbrignull robertbrignull Oct 11, 2023

Choose a reason for hiding this comment

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

Yep, I agree it's not the prettiest and clearest. I'll extract the whole thing to a method perhaps. The main reason for it being one ternary is so we can keep the variable as a const, so that's why I'm suggesting something other than splitting into two steps.

extensions/ql-vscode/src/view/model-editor/MethodRow.tsx Outdated Show resolved Hide resolved
@robertbrignull
Copy link
Contributor Author

Existing tests are now fixed. I'll look at adding some more for MethodRow for editing multiple models.

@robertbrignull
Copy link
Contributor Author

I've added a couple more tests of MethodRow. If there's anything else that's sensible to test at this stage let me know, but I couldn't think of more things until we add the controls for adding/removing models.

@robertbrignull robertbrignull merged commit 1993db5 into main Oct 12, 2023
@robertbrignull robertbrignull deleted the robertbrignull/save_multiple_modeled_methods branch October 12, 2023 08:19
@koesie10 koesie10 mentioned this pull request Oct 12, 2023
3 tasks
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