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.

Sign up for GitHub

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

Jump to bottom

Introduce localize2 function #194750

Merged
merged 3 commits into from
Oct 4, 2023
Merged

Introduce localize2 function #194750

merged 3 commits into from
Oct 4, 2023

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Oct 3, 2023 •
edited
Loading

This is syntax sugar around:

{ value localize('id', "Hello"), original: 'Hello' }

That will now be returned when you do:

localize2('id', "Hello");

I tested this with a full build to ensure rewriting works as expected.

@TylerLeonhardt TylerLeonhardt enabled auto-merge (squash) October 3, 2023 21:06
@TylerLeonhardt TylerLeonhardt self-assigned this Oct 3, 2023
@vscodenpa vscodenpa added this to the October 2023 milestone Oct 3, 2023
alexdima
alexdima previously approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

Looks good to me! I would suggest to try it out with a full build before merging.

@TylerLeonhardt
Introduce localize2 function
400529e 
This is syntax sugar around:
```
{ value localize('id', "Hello"), original: 'Hello' }
```

That will now be returned when you do:
```
localize2('id', "Hello");
```
@TylerLeonhardt
fix merge conflic
cba0802
@connor4312
Copy link
Member

connor4312 commented Oct 4, 2023 •
edited
Loading

Maybe bikeshedding, but is "localize2" the right name for this? To me 2 implies a temporary name in a transitionary period until everyone is using the new one -- i.e. the non-2 version is deprecated. But ILocalizedString is only for use in certain areas like commands (or is it?)

connor4312
connor4312 previously approved these changes Oct 4, 2023
View reviewed changes
@TylerLeonhardt
Copy link
Member Author

Connor mentioned localizeCommand and localizeLabel as alternatives. localizeCommand doesn't really work because ILocalizeString is used in View names as well.

And localizeLabel ehh I don't like it enough to change it... plus localize2 is not a lot of additional characters.

Also, I think we can make bigger changes to the API when we align core with extension's vscode.l10n.t().

@TylerLeonhardt TylerLeonhardt enabled auto-merge (squash) October 4, 2023 19:44
@TylerLeonhardt TylerLeonhardt merged commit a27dc77 into main Oct 4, 2023
6 checks passed
@TylerLeonhardt TylerLeonhardt deleted the tyler/domestic-emu branch October 4, 2023 19:57
@TylerLeonhardt TylerLeonhardt mentioned this pull request Oct 4, 2023
Merged
Alex0007 pushed a commit to Alex0007/vscode that referenced this pull request Oct 26, 2023
@TylerLeonhardt @Alex0007
Introduce localize2 function (microsoft#194750)
27aa7c8 
* Introduce `localize2` function

This is syntax sugar around:
```
{ value localize('id', "Hello"), original: 'Hello' }
```

That will now be returned when you do:
```
localize2('id', "Hello");
```

* fix merge conflic

* new source map due to updated deps
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@connor4312 connor4312 connor4312 approved these changes

@alexdima alexdima alexdima left review comments

@jrieken jrieken Awaiting requested review from jrieken

Assignees

@TylerLeonhardt TylerLeonhardt

Labels
None yet
Projects
None yet
Milestone
October 2023
Development

Successfully merging this pull request may close these issues.
4 participants
@TylerLeonhardt @connor4312 @alexdima @vscodenpa