Modify formatting of perference's name to align vscode

[shuyaqian]

Signed-off-by: shuyaqian [email protected]

What it does

Avoid the problem that if the perference's name contains Chinese characters or special characters like C++ will be filtered.

How to test

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[colin-grant-work]

@shuyaqian, thank you for your contribution! With your changes, I'm seeing some problems where names that should be split are not being split:

You mention in your PR description that the problem occurs with Chinese characters and special characters like the + in C++. Could you add some tests to the test suite for the various cases?

For example completePropertyWithSemicolon should become Complete Property With Semicolon but C++ should remain C++ and 某個設定/某个设定 should remain 某個設定/某个设定.

[shuyaqian]

@shuyaqian, thank you for your contribution! With your changes, I'm seeing some problems where names that should be split are not being split:

You mention in your PR description that the problem occurs with Chinese characters and special characters like the + in C++. Could you add some tests to the test suite for the various cases?

For example completePropertyWithSemicolon should become Complete Property With Semicolon but C++ should remain C++ and 某個設定/某个设定 should remain 某個設定/某个设定.

Thanks, I've revised it.

[colin-grant-work]

Thanks, I've revised it.

Thank you very much! One thing I notice is that the new regex uses ranges like [a-z] to a lot. That means it now works well for the vanilla Latin alphabet, but it wouldn’t work for any other alphabets. To generalize it, I think it may be better to apply an iterative approach, rather than a regex, something like this (pseudocode):

Initialize output to empty string
Iterate over input string
If the first character can be capitalized, capitalize it.
If you encounter a capitalized character following a non-capitalized character, add a space before the capitalized character.
Otherwise just add the character

e.g.
Danish:
indstillingPåEnØ -> Indstilling På En Ø

Greek:
κάποιαΡύθμιση -> Κάποια Ρύθμιση

Russian:
некоторыеНастройки -> Некоторые Настройки

Armenian:
ինչ-որՊարամետր -> Ինչ-որ Պարամետր

I haven’t tested every case, but JavaScript’s toUpperCase() and toLowerCase() methods work on a wide variety of character sets, so a check like a.toLowerCase() !== a works pretty well to tell if something is capitalized, and a.toUpperCase() !== a works pretty well to check whether something could be capitalized. I’ve got some code here that works on the Armenian example, and should cover the rest, though it can certainly be improved.

Punctuation is a challenge. For example something like hyphenated-wordC++Setting should maybe become Hyphenated-word C++ Setting, but I don’t think we have a mechanism at the moment for making any useful distinctions there. I think we should probably allow something like a displayName in the preference schema and only fall back to calculating it if that doesn’t exist, but that’s outside the scope of this PR.

[shuyaqian]

Thanks, I've revised it.

Thank you very much! One thing I notice is that the new regex uses ranges like [a-z] to a lot. That means it now works well for the vanilla Latin alphabet, but it wouldn’t work for any other alphabets. To generalize it, I think it may be better to apply an iterative approach, rather than a regex, something like this (pseudocode):

Initialize output to empty string
Iterate over input string
If the first character can be capitalized, capitalize it.
If you encounter a capitalized character following a non-capitalized character, add a space before the capitalized character.
Otherwise just add the character

e.g.
Danish:
indstillingPåEnØ -> Indstilling På En Ø

Greek:
κάποιαΡύθμιση -> Κάποια Ρύθμιση

Russian:
некоторыеНастройки -> Некоторые Настройки

Armenian:
ինչ-որՊարամետր -> Ինչ-որ Պարամետր

I haven’t tested every case, but JavaScript’s toUpperCase() and toLowerCase() methods work on a wide variety of character sets, so a check like a.toLowerCase() !== a works pretty well to tell if something is capitalized, and a.toUpperCase() !== a works pretty well to check whether something could be capitalized. I’ve got some code here that works on the Armenian example, and should cover the rest, though it can certainly be improved.

Punctuation is a challenge. For example something like hyphenated-wordC++Setting should maybe become Hyphenated-word C++ Setting, but I don’t think we have a mechanism at the moment for making any useful distinctions there. I think we should probably allow something like a displayName in the preference schema and only fall back to calculating it if that doesn’t exist, but that’s outside the scope of this PR.

I have a preference for 'SecSolar', which is an acronym, and I don't want it to be 'Sec Solar', but in VSCODE it's also 'Sec Solar'. I think this can be aligned with vscode and ignored for the time being. The rest of the logic is executed according to the pseudocode above.

[colin-grant-work]

The code looks good and the tests are a very nice addition, only very minor issues remaining.

[colin-grant-work]

Suggested change

	private formatString(string: string): string {
	protected formatString(string: string): string {

[colin-grant-work]

Suggested change

	private isUpperCase(char: string): boolean {
	protected isUpperCase(char: string): boolean {

[colin-grant-work]

Oops, a test is now failing. Looks like it expects capitalization after white space, as well as string-initially. I think that expectation is incorrect. If you create a preference name in the following form:

"references.My perfectly formatted Display Name"`

VSCode does not capitalize the words after spaces:

So you can simply change the expected output.

[shuyaqian]

Thanks, I've revised it. I don't know why the test cases don't run automatically after force-push🤪，maybe need you to approve.

[colin-grant-work]

@shuyaqian, looks like it did re-run, and there's still a tiny problem: the current code is adding a space in the sequence space + capital:

@theia/preferences:   1) preference-tree-generator
@theia/preferences:        PreferenceTreeGenerator.format:
@theia/preferences:       AssertionError: expected 'Aaa Bbb Ccc  Dddd eee' to equal 'Aaa Bbb Ccc Dddd eee'
@theia/preferences:       + expected - actual
@theia/preferences:       -Aaa Bbb Ccc  Dddd eee      (colin-grant-work: two spaces in 'Ccc  Dddd' here instead of the expected one)
@theia/preferences:       +Aaa Bbb Ccc Dddd eee

You can also run the tests locally: from the packages/preferences directory, run yarn test, and it will run the tests for that package.

[shuyaqian]

@shuyaqian, looks like it did re-run, and there's still a tiny problem: the current code is adding a space in the sequence space + capital:

@theia/preferences:   1) preference-tree-generator
@theia/preferences:        PreferenceTreeGenerator.format:
@theia/preferences:       AssertionError: expected 'Aaa Bbb Ccc  Dddd eee' to equal 'Aaa Bbb Ccc Dddd eee'
@theia/preferences:       + expected - actual
@theia/preferences:       -Aaa Bbb Ccc  Dddd eee      (colin-grant-work: two spaces in 'Ccc  Dddd' here instead of the expected one)
@theia/preferences:       +Aaa Bbb Ccc Dddd eee

You can also run the tests locally: from the packages/preferences directory, run yarn test, and it will run the tests for that package.

I ignored the space in front of the uppercase letter, and I modified the criteria, now it's okay.

[colin-grant-work]

Tests are passing and the code looks good. Thanks for the contribution and for the tests, @shuyaqian!

[colin-grant-work]

Minor: I think this is fine as is - I'll approve it momentarily - but the check !== ' ' could be generalized to regex /\s/, since not all scripts use the ' ' character.

[colin-grant-work]

I will merge this tomorrow, if there are no objections.

[shuyaqian]

I will merge this tomorrow, if there are no objections.

Thanks, hopefully this will end the PR this time😅

[colin-grant-work]

Thanks, hopefully this will end the PR this time😅

Thanks for going along with my expansion of the scope of the PR. I think this is a big improvement over the previous implementation.