Skip to content

Convert SetSelectedMethodMessage to include multiple modeled methods#2947

Merged
koesie10 merged 3 commits intomainfrom
koesie10/update-set-selected-method-message
Oct 11, 2023
Merged

Convert SetSelectedMethodMessage to include multiple modeled methods#2947
koesie10 merged 3 commits intomainfrom
koesie10/update-set-selected-method-message

Conversation

@koesie10
Copy link
Copy Markdown
Member

This converts the SetSelectedMethodMessage to include multiple methods. It also adds a new message to replicate the SetModeledMethodMessage with multiple modeled methods so that all state is now pushed through correctly from the modeled method view provider. Unfortunately, the name SetModeledMethodsMessage is already taken by the modeled methods map, so I've created a new message SetMultipleModeledMethodsMessage. We may want to consider changing these once all messages are properly renamed.

The changes to canMethodBeModeled are directly copied from https://github.com/github/vscode-codeql/pull/2942/files#diff-fb5d586d735e8d18fcae2a4225ba24aff4416e9423ea1a4fb1af7bc5d5805da8

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 11, 2023 09:02
@koesie10 koesie10 requested review from a team as code owners October 11, 2023 09:02
Copy link
Copy Markdown
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.

LGTM

Comment thread extensions/ql-vscode/src/common/interface-types.ts

interface SetMultipleModeledMethodsMessage {
t: "setMultipleModeledMethods";
modeledMethods: ModeledMethod[];
Copy link
Copy Markdown
Contributor

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

Did we discuss including methodSignature: string; in this message, so you don't have to work out the signature by looking at the modeled methods?

That would also make it easier to remove all modeled methods for a method signature, instead of having to set it to the array containing one element with type: "none".

I'm happy for you to make this change in this PR or I can do it in a followup, because I'll be using this message in my next PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It does make sense to add it here. Right now, this message is only used for communication from the extension host to the view, but we'll definitely need the signature for the communication from the view to the extension host. I'll change the message now so that I can also use it in a follow-up PR (after #2924).

Copy link
Copy Markdown
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.

Thanks for updating and adding methodSignatures.

Has conflicts, but otherwise LGTM

@koesie10 koesie10 enabled auto-merge October 11, 2023 11:55
@koesie10 koesie10 merged commit 3a03570 into main Oct 11, 2023
@koesie10 koesie10 deleted the koesie10/update-set-selected-method-message branch October 11, 2023 11:59
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