Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for creating and updating locales #480

Merged
merged 11 commits into from
Dec 15, 2023

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented Nov 21, 2023

This PR adds two new commands new-locale and update-locale following the specifications listed in issue #78.

Locale comparisons

For locale comparions, I've used RegionInfo Class (reference: https://learn.microsoft.com/en-us/dotnet/api/system.globalization.regioninfo?view=net-7.0&redirectedfrom=MSDN). The benefit here is that it validates the locale format and also treats locales such as en-US, en-USA, en-us etc as same which prevents duplicate locales added in the manifest.

### Command flow shift

For now, I've only added supporting for shifting from new-locale to update-locale and vice-versa when --locale argument is provided. My thinking here is that since both the commands support creating/updating multiple locales, what if the user enters a non-existing locale after updating multiple locales? User may expect the generated PR to contain both updated and newly created locales, but I've not handled that case here.

Let me know if I should add the ability to prompt for flow change whenever an invalid locale is input by the user (or only the first time user inputs an invalid locale?). Removed (see #480 (comment))


Microsoft Reviewers: Open in CodeFlow

@mdanish-kh mdanish-kh requested a review from a team as a code owner November 21, 2023 07:04
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team November 21, 2023 07:04
@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 21, 2023
Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

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

Apologies for the delay 😅 This is a really cool feature so it took me some time to get through all of it since we don't have very good tests for prompting.

doc/new-locale.md Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/NewLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/NewLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/LocaleHelper.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@mdanish-kh mdanish-kh left a comment

Choose a reason for hiding this comment

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

I've resolved many of the review comments but left the ones out that require having #480 (comment) resolved first and ones requiring additional feedback

src/WingetCreateCLI/Commands/BaseCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/NewLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/NewLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/LocaleHelper.cs Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateLocaleCommand.cs Outdated Show resolved Hide resolved
@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

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

Super awesome, thanks for working on this!

@ryfu-msft ryfu-msft merged commit 445ffa2 into microsoft:main Dec 15, 2023
3 of 5 checks passed
@mdanish-kh mdanish-kh deleted the new-update-locale branch December 15, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify localization for existing packages
2 participants