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

fix: to #157015 fix view label command localized #193544

Merged
merged 22 commits into from
Oct 10, 2023

Conversation

yiliang114
Copy link
Contributor

@yiliang114 yiliang114 commented Sep 20, 2023

Fixes #157015

@TylerLeonhardt
Copy link
Member

Awesome!

Looks like you do have a couple of issues still:

[tsec-compile-check      ] src/vs/workbench/contrib/scm/browser/scm.contribution.ts(115,2): error TS2322: Type 'string' is not assignable to type 'ILocalizedString'.
[tsec-compile-check      ] src/vs/workbench/contrib/userDataSync/browser/userDataSyncViews.ts(181,32): error TS2352: Conversion of type '{ id: string; name: string; ctorDescriptor: SyncDescriptor<TreeViewPane>; when: RawContextKey<boolean>; canToggleVisibility: true; canMoveView: false; treeView: TreeView; collapsed: false; hideByDefault: false; }' to type 'ITreeViewDescriptor' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
[tsec-compile-check      ]   Types of property 'name' are incompatible.
[tsec-compile-check      ]     Type 'string' is not comparable to type 'ILocalizedString'.
[tsec-compile-check      ] 
[tsec-compile-check      ] 
[tsec-compile-check      ] Found 2 errors.
[tsec-compile-check      ] error Command failed with exit code 1.

From the CI

@TylerLeonhardt TylerLeonhardt added this to the Backlog milestone Sep 21, 2023
@yiliang114
Copy link
Contributor Author

Awesome!

Looks like you do have a couple of issues still:

[tsec-compile-check      ] src/vs/workbench/contrib/scm/browser/scm.contribution.ts(115,2): error TS2322: Type 'string' is not assignable to type 'ILocalizedString'.
[tsec-compile-check      ] src/vs/workbench/contrib/userDataSync/browser/userDataSyncViews.ts(181,32): error TS2352: Conversion of type '{ id: string; name: string; ctorDescriptor: SyncDescriptor<TreeViewPane>; when: RawContextKey<boolean>; canToggleVisibility: true; canMoveView: false; treeView: TreeView; collapsed: false; hideByDefault: false; }' to type 'ITreeViewDescriptor' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
[tsec-compile-check      ]   Types of property 'name' are incompatible.
[tsec-compile-check      ]     Type 'string' is not comparable to type 'ILocalizedString'.
[tsec-compile-check      ] 
[tsec-compile-check      ] 
[tsec-compile-check      ] Found 2 errors.
[tsec-compile-check      ] error Command failed with exit code 1.

From the CI

Thanks!

Now it is reasonable to say that all the processing has been completed.
image

(I think I should have handled all the compilation errors before. When I update the main branch code, some code may be restored due to automatic merging)

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

This is super close! Just a few comments

@TylerLeonhardt
Copy link
Member

FYI we are currently in the middle of a release right now, so I want to hold off on getting this in til next week. That way we can let it sit in Insiders for a bit to make sure we don't cause any regressions due to the number of files changed. I think the PR is ready, but I'll approve and merge early next week.

@TylerLeonhardt TylerLeonhardt modified the milestones: Backlog, October 2023 Sep 27, 2023
@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Oct 4, 2023

@yiliang114 I hope you don't hate me for this but I just added a new thing that will make this PR much smaller and have less duplicate strings.

I have added a localize2 function in this PR that allows us to replace all of these:

{ value: nls.localize({ comment: ['Debug is a noun in this context, not a verb.'], key: 'debugPanel' }, "Debug Console"), original: 'Debug Console' }

with a much simpler:

nls.localize2({ comment: ['Debug is a noun in this context, not a verb.'], key: 'debugPanel' }, "Debug Console")

this function returns an ILocalizedString and handles the original string correctly.

Can you move over to using that so the code is easier to maintain and we don't need to have two of the same string next to each other?

I think this won't apply to everything in your PR but probably about 80-90% if the cases.

@yiliang114
Copy link
Contributor Author

@yiliang114 I hope you don't hate me for this but I just added a new thing that will make this PR much smaller and have less duplicate strings.

I have added a localize2 function in this PR that allows us to replace all of these:

{ value: nls.localize({ comment: ['Debug is a noun in this context, not a verb.'], key: 'debugPanel' }, "Debug Console"), original: 'Debug Console' }

with a much simpler:

nls.localize2({ comment: ['Debug is a noun in this context, not a verb.'], key: 'debugPanel' }, "Debug Console")

this function returns an ILocalizedString and handles the original string correctly.

Can you move over to using that so the code is easier to maintain and we don't need to have two of the same string next to each other?

I think this won't apply to everything in your PR but probably about 80-90% if the cases.

Haha, it looks better this way. Let me try again.

@yiliang114
Copy link
Contributor Author

Please help me review the code again when you are free @TylerLeonhardt , I did not encountered an error when I compiled locally.

image

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM thanks for taking this on!

@TylerLeonhardt TylerLeonhardt enabled auto-merge (squash) October 9, 2023 22:34
@@ -63,13 +64,13 @@ suite('ViewContainerModel', () => {
});

test('empty model', function () {
container = ViewContainerRegistry.registerViewContainer({ id: 'test', title: { value: 'test', original: 'test' }, ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
container = ViewContainerRegistry.registerViewContainer({ id: 'test', title: nls.localize2('test', 'test'), ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
Copy link
Member

Choose a reason for hiding this comment

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

These will be excluded from the actual localization process, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

@TylerLeonhardt TylerLeonhardt merged commit eb4f3c1 into microsoft:main Oct 10, 2023
6 checks passed
TylerLeonhardt added a commit that referenced this pull request Oct 16, 2023
TylerLeonhardt added a commit that referenced this pull request Oct 16, 2023
Alex0007 pushed a commit to Alex0007/vscode that referenced this pull request Oct 26, 2023
…193544)

* fix: to microsoft#157015 fix view label command localized

* chore: save

* Fixes microsoft#157015

* Fixes microsoft#157015

* Fixes microsoft#157015

* Fixes microsoft#157015

* Fixes microsoft#157015

* Fixes microsoft#157015

* fix: use nls.localize2 to simplified code

* fix: use nls.localize2 to simplified code

* fix: use nls.localize2 to simplified code

---------

Co-authored-by: Johannes Rieken <johannes.rieken@gmail.com>
Co-authored-by: Isidor Nikolic <inikolic@microsoft.com>
Co-authored-by: Megan Rogge <merogge@microsoft.com>
Co-authored-by: Ulugbek Abdullaev <ulugbekna@gmail.com>
Alex0007 pushed a commit to Alex0007/vscode that referenced this pull request Oct 26, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus on __ View commands are improperly localized
9 participants