Skip to content

Fix typo and remove unnecessary eslint comments #4055

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

Merged
Tyriar merged 4 commits into from
Aug 24, 2022
Merged

Fix typo and remove unnecessary eslint comments #4055

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b59de22
fix typo
GreenMashimaro Aug 23, 2022
559b695
remove unnecessary eslint comments
GreenMashimaro Aug 23, 2022
ac81d3f
feat: restore LocalizableStrings.ts const export
GreenMashimaro Aug 24, 2022
86ba6cd
Tweak comment
Tyriar Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/browser/LocalizableStrings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,5 @@
* @license MIT
*/

// eslint-disable-next-line prefer-const
export let promptLabel = 'Terminal input';

// eslint-disable-next-line prefer-const
export let tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
Comment on lines -6 to -10
Copy link
Copy Markdown
Member

@Tyriar Tyriar Aug 23, 2022

Choose a reason for hiding this comment

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

This actually needs to be let, it's exposed to embedders of xterm.js so they can change the strings to localize xterm.js. It's exposed (as non-readonly) here

xterm.js/src/browser/public/Terminal.ts

Lines 242 to 244 in c0ef2cc

public static get strings(): ILocalizableStrings {
return Strings;
}

Copy link
Copy Markdown
Contributor Author

@GreenMashimaro GreenMashimaro Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your reply.

I verified it myself within the project. Validation does not affect the original functional logic.

Verify 1: From the compilation results, the two are the same.

original code

export let promptLabel = 'Terminal input';
export let tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';

after compilation code

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.tooMuchOutput = exports.promptLabel = void 0;
exports.promptLabel = 'Terminal input';
exports.tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
//# sourceMappingURL=LocalizableStrings.js.map

original code

export const promptLabel = 'Terminal input';
export const tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';

after compilation code

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.tooMuchOutput = exports.promptLabel = void 0;
exports.promptLabel = 'Terminal input';
exports.tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
//# sourceMappingURL=LocalizableStrings.js.map

Verify 2: Use let to declare variables; the value cannot be modified in the import place. The two are the same.

image

Verify 3: The exported const variable can also be modified normally

export const promptLabel = 'Terminal input';
export const tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';

and then, no error prompt

Terminal.strings.promptLabel = 'modify';

Copy link
Copy Markdown
Member

@Tyriar Tyriar Aug 24, 2022

Choose a reason for hiding this comment

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

Ah I see, still using const shows the wrong intent here. const means this value should not change, and while currently TypeScript outputs is as mutable, it may not in the future. So I think we should keep this as it was

Copy link
Copy Markdown
Contributor Author

@GreenMashimaro GreenMashimaro Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think your idea is good, and I suggest adding a comment. I re-updated the submission.

export const promptLabel = 'Terminal input';
export const tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
4 changes: 2 additions & 2 deletions src/common/buffer/BufferSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ export class BufferSet extends Disposable implements IBufferSet {
}

/**
* Returns the normal Buffer of the BufferSet
* Returns the currently active Buffer of the BufferSet
*/
public get active(): Buffer {
return this._activeBuffer;
}

/**
* Returns the currently active Buffer of the BufferSet
* Returns the normal Buffer of the BufferSet
*/
public get normal(): Buffer {
Comment thread
GreenMashimaro marked this conversation as resolved.
return this._normal;
Expand Down