prompt: case-insensitive sorting for subscriptions #4969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weikanglim
requested review from
wbreza,
vhvb1989,
hemarina and
JeffreyCA
as code owners
JeffreyCA
previously approved these changes
Mar 19, 2025
vhvb1989
previously approved these changes
Mar 19, 2025
rajeshkamal5050
previously approved these changes
Mar 19, 2025
weikanglim
commented
Mar 21, 2025
cli/azd/pkg/prompt/prompter.go
Outdated
| @@ -218,6 +219,10 @@ func (p *DefaultPrompter) getSubscriptionOptions(ctx context.Context) ([]string, | |||
| return nil, nil, nil, fmt.Errorf("listing accounts: %w", err) | |||
| } | |||
|
|
|||
| sort.Slice(subscriptionInfos, func(i, j int) bool { | |||
| return strings.ToLower(subscriptionInfos[i].Name) < strings.ToLower(subscriptionInfos[j].Name) | |||
There was a problem hiding this comment.
The calls to strings.ToLower here ends up allocating 2x string copies for each element.
A way to avoid string allocations is to leverage slices and lowercasing each character rune. This kind of string comparison doesn't work for all languages -- but it would work for ASCII usage.
weikanglim
force-pushed
the
weilim/sort-subscriptions
branch
from
bc29144 to
105b26c
Compare
weikanglim
commented
Mar 21, 2025
| @@ -157,7 +157,7 @@ func (p *DefaultPrompter) PromptResourceGroupFrom( | |||
| } | |||
|
|
|||
| slices.SortFunc(groups, func(a, b *azapi.Resource) int { | |||
| return strings.Compare(a.Name, b.Name) | |||
| return stringutil.CompareLower(a.Name, b.Name) | |||
There was a problem hiding this comment.
@vhvb1989 , @JeffreyCA made a really good point on the linked issue that resource groups should behave similarly.
I suspect it goes beyond just these resources, so I added a new comparison function that avoids string allocations (previous implementation used strings.ToLower which creates a new string copy per each element iteration), if we wanted to do this more generally.
However, I'm truly wondering if we're down the right track here. There are many different Azure resources that will likely encounter the same problem, and I'm not sure if we'll want to apply this rule consistently.
Does the case-insensitive comparison truly feel any better? Presentation-wise it can get look a little messy at times with mixed capitalization. Curious what everyone thinks here -- My position shifted to slightly against this after looking at it more.
There was a problem hiding this comment.
As long as the output is deterministic, I don't have any strong opinion about the sorting details.
Similar prompts are: Key vault Accounts and Secrets (or maybe not due to naming-rules for key vault), AI Models listing, AI Projects listing, etc.
There was a problem hiding this comment.
I wouldn't have any objections with just an easy normalized to lower based sorting instead of something more complex considering any lists we show are relatively small (< 100 items).
There was a problem hiding this comment.
To me yes it feels a little more natural than listing names starting with A-Z followed by a-z. I found this resource naming doc, which I don't know if it's helpful. Most names seem to be case-sensitive, but it varies quite a bit whether uppercase letters are permitted in the name
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
weikanglim
dismissed stale reviews from rajeshkamal5050, vhvb1989, and JeffreyCA
via
105b26c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apply case-insensitive sorting at the subscription prompt display
Fixes #4961