Skip to content

Conversation

dustinbyrne
Copy link
Contributor

This implements the requested changes in #1053

Copy link
Contributor

@dividedmind dividedmind left a comment

Choose a reason for hiding this comment

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

Looks ok, thanks! One thing that's bugging me: won't calling refreshModels() here https://github.com/getappmap/vscode-appland/pull/1055/files#diff-7f9409f608e34972dc33bd02b1bb6d84fb8d0b412cbd6d3d4d01d054a1da9fb6R18 potentially cause it to miss a change in the auto-chosen model (in case the list changed in the meantime and refreshModels() wasn't called before for some reason, eg. due to a race)? I suppose it's unlikely to ever happen, but maybe it's more correct not to call it there at all (and make it private for good measure, although that might complicate tests).

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