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

Describe defaults of more options #46498

Merged
merged 3 commits into from
Dec 1, 2021
Merged

Describe defaults of more options #46498

merged 3 commits into from
Dec 1, 2021

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Oct 23, 2021 •
edited
Loading

Adds defaultValueDescription to:

  • importsNotUsedAsValues
  • watchFile
  • watchDirectory
  • fallbackPolling

/cc @orta

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 23, 2021
@jablko jablko force-pushed the patch-41 branch 6 times, most recently from b665e67 to 3041c01 Compare October 24, 2021 21:18
jablko
jablko commented Oct 24, 2021
View reviewed changes
src/compiler/types.ts
}

/* @internal */
export interface CommandLineOptionOfCustomType extends CommandLineOptionBase {
type: ESMap<string, number | string>; // an object literal mapping named values to actual values
defaultValueDescription: number | string | undefined | DiagnosticMessage;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now added a commit to accommodate enum members vs. strings in defaultValueDescription, e.g.

TypeScript/src/compiler/commandLineParser.ts

Line 596 in d31ac53

defaultValueDescription: ImportsNotUsedAsValues.Remove,

src/compiler/types.ts
}

/* @internal */
export interface CommandLineOptionOfBooleanType extends CommandLineOptionBase {
type: "boolean";
defaultValueDescription: `${boolean | undefined}` | DiagnosticMessage;
defaultValueDescription: boolean | undefined | DiagnosticMessage;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and likewise number/boolean literals vs. strings.

src/executeCommandLine/executeCommandLine.ts
.filter(([, value]) => value === defaultValue)
.map(([name]) => name)
.join("/")
: String(defaultValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated --help to stringify enum members and number/boolean literals, e.g. ImportsNotUsedAsValues.Remove -> remove.

src/executeCommandLine/executeCommandLine.ts
});
return getEntries(inverted)
.map(([, synonyms]) => synonyms.join("/"))
.join(", ");
Copy link
Contributor Author

@jablko jablko Oct 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Because this prints all of an enum member's synonyms, I correspondingly updated the way --help formats possible values. Not that any current default has synonyms.

...elines/reference/tsc/runWithoutArgs/initial-build/does-not-add-color-when-NO_COLOR-is-set.js Outdated
@@ -68,16 +68,18 @@ default: true

--target, -t
Set the JavaScript language version for emitted JavaScript and include compatible library declarations.
one of: es3, es5, es6, es2015, es2016, es2017, es2018, es2019, es2020, es2021, esnext
default: ES3
one of: es3, es5, es6/es2015, es2016, es2017, es2018, es2019, es2020, es2021, esnext
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i.e. es6, es2015 -> es6/es2015

@jablko jablko marked this pull request as ready for review October 24, 2021 22:23
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@sandersn sandersn requested a review from orta November 17, 2021 23:45
@sandersn
Copy link
Member

This PR seems to do a lot more than add default value descriptions. Can you add more explanation to cover the rest of the changes?

@jablko jablko changed the title Describe defaults of some watch options Describe defaults of more options Nov 18, 2021
@jablko
Copy link
Contributor Author

jablko commented Nov 18, 2021

👍 Can do:

  1. In 9c2740b I added defaultValueDescriptions to importsNotUsedAsValues, watchFile, watchDirectory and fallbackPolling.
  2. I revised the CommandLineOptionBase and CommandLineOptionOfCustomType types, in 61b07a3#diff-e9fd483341eea176a38fbd370590e1dc65ce2d9bf70bfd317c5407f04dba9560R6376, to accommodate enum members (e.g. defaultValueDescription: ImportsNotUsedAsValues.Remove) and analogously revised CommandLineOptionOfNumberType and CommandLineOptionOfBooleanType from strings -> numbers/booleans (e.g. `${number | undefined}` -> number | undefined.
  3. I updated --help (61b07a3#diff-5bc64df2f0c1746eae137760ddd218696b382812e3b408d417bd838f9620a75dR212) to stringify enum members (ImportsNotUsedAsValues.Remove -> remove vs. e.g. 0).

@sandersn sandersn requested a review from andrewbranch December 1, 2021 00:35
orta
orta approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Thanks for the writeup, this looks good to me and I think it improves the CLI UX 👍🏻

@sandersn sandersn merged commit 670ad45 into microsoft:main Dec 1, 2021
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@jablko @typescript-bot @mprobst
Describe defaults of more options (microsoft#46498)
0cc3521 
* Describe defaults of more options

* Use enum members/values vs. strings

* Update Baselines and/or Applied Lint Fixes

Co-authored-by: TypeScript Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orta orta orta approved these changes

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

Assignees

@orta orta

Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jablko @typescript-bot @sandersn @orta