Skip to content

Refresh subscription instead of group#618

Merged
alexweininger merged 2 commits intomainfrom
alex/brown-haddock
Mar 22, 2023
Merged

Refresh subscription instead of group#618
alexweininger merged 2 commits intomainfrom
alex/brown-haddock

Conversation

@alexweininger
Copy link
Copy Markdown
Member

@alexweininger alexweininger commented Mar 20, 2023

Fixes #617

@alexweininger alexweininger requested a review from a team as a code owner March 20, 2023 19:27
@alexweininger alexweininger mentioned this pull request Mar 20, 2023
Closed
nturinski
nturinski reviewed Mar 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

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

Could we make the refresh implementation different on the node itself rather than having this weird edge case in the command defintion?

Like, if you call GroupingTreeItem's refresh, it's implementation will handle passing up the subscription/parent node? Or do we not have that capability anymore?

@alexweininger
Copy link
Copy Markdown
Member Author

alexweininger commented Mar 20, 2023

Could we make the refresh implementation different on the node itself rather than having this weird edge case in the command defintion?

Like, if you call GroupingTreeItem's refresh, it's implementation will handle passing up the subscription/parent node? Or do we not have that capability anymore?

We haven't done it that way for v2, however that's probably a good approach.

@nturinski
Copy link
Copy Markdown
Member

nturinski commented Mar 21, 2023

Is it possible to do that now or should I just go ahead and approve this for merge, and we can leave an issue for it later?

@alexweininger
Copy link
Copy Markdown
Member Author

alexweininger commented Mar 21, 2023

Is it possible to do that now or should I just go ahead and approve this for merge, and we can leave an issue for it later?

I will change it sometime today (hopefully). 😄

@alexweininger
Copy link
Copy Markdown
Member Author

alexweininger commented Mar 22, 2023 •
edited
Loading

Since this is sorta a special case, I think for now this implementation is fine. None of the solutions I have feel right since it's just for this one edge case.

@nturinski
Copy link
Copy Markdown
Member

nturinski commented Mar 22, 2023

Alright, definitely something to keep in mind when we convert a hefty boy like Functions.

@alexweininger alexweininger merged commit 0c208b7 into main Mar 22, 2023
@alexweininger alexweininger deleted the alex/brown-haddock branch March 22, 2023 23:03
bwateratmsft
bwateratmsft reviewed Mar 23, 2023
View reviewed changes
Comment thread src/commands/registerCommands.ts
ext.actions.refreshAzureTree(node);

if (node instanceof GroupingItem) {
ext.actions.refreshAzureTree(node.parent);
Copy link
Copy Markdown
Contributor

@bwateratmsft bwateratmsft Mar 23, 2023

Choose a reason for hiding this comment

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

(Too late now but) can we add a comment explaining why?

@alexweininger alexweininger mentioned this pull request Mar 23, 2023
Merged
@microsoft microsoft locked and limited conversation to collaborators May 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bwateratmsft bwateratmsft bwateratmsft left review comments

@nturinski nturinski nturinski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refreshing a group item doesn't behave as expected

3 participants

@alexweininger @nturinski @bwateratmsft
⚡