Skip to content

Convert SetModeledMethodsMessage to include multiple modeled methods#2942

Merged
robertbrignull merged 3 commits intomainfrom
robertbrignull/SetModeledMethodsMessage
Oct 11, 2023
Merged

Convert SetModeledMethodsMessage to include multiple modeled methods#2942
robertbrignull merged 3 commits intomainfrom
robertbrignull/SetModeledMethodsMessage

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Converts SetModeledMethodsMessage to include multiple modeled methods and pushes that change throughout the model editor. This means it's now possible to view multiple methods, though it's not yet possible to edit them and save the changes.

This can be tested by manually editing a model file to give multiple modelings for the same method.

Screenshot 2023-10-10 at 15 44 08

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 10, 2023 14:44
Base automatically changed from robertbrignull/CommonFromViewMessages to main October 10, 2023 14:49
@robertbrignull robertbrignull force-pushed the robertbrignull/SetModeledMethodsMessage branch from c569e34 to c6a9f23 Compare October 10, 2023 14:50
libraryVersion?: string;
methods: Method[];
modeledMethods: Record<string, ModeledMethod>;
modeledMethodsMap: Record<string, ModeledMethod[]>;
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.

Is there a specific reason for using modeledMethodsMap here, while we're using modeledMethods everywhere else?

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

My intention was to change all instances of things with the type Record<string, ModeledMethod[]> in the model editor webview to modeledMethodsMap, so they're all consistent. That way we aren't calling the data modeledMethods in one component, but then passing it to another component where we call the same data modeledMethodsMap and modeledMethods has a different meaning.

Do you think we should just call it modeledMethods unless we also happen to need objects of type ModeledMethod[] in the same scope?

I didn't spot that my automated renaming had renamed the fields in the type but not when they're used in the component. So we have modeledMethodsMap: modeledMethods when we get the value from the props. I'll fix that because I think that's definitely unhelpful, regardless of the decision above.

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, I think the modeledMethodsMap: modeledMethods was just confusing to me.

@robertbrignull robertbrignull merged commit c459d0f into main Oct 11, 2023
@robertbrignull robertbrignull deleted the robertbrignull/SetModeledMethodsMessage branch October 11, 2023 10:45
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