Skip to content

Remove defaultValueDescription: "n/a" from options #46280

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
Merged
merged 3 commits into from
Oct 19, 2021
Merged

Remove defaultValueDescription: "n/a" from options #46280

merged 3 commits into from
Oct 19, 2021

Conversation

@jablko
Copy link
Contributor

@jablko jablko commented Oct 8, 2021

Are there different meanings to defaultValueDescription: "n/a" and omitting defaultValueDescription?

/cc @orta

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 8, 2021
@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 8, 2021

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

jablko
jablko commented Oct 9, 2021
View reviewed changes
src/compiler/types.ts Outdated
/* @internal */
export interface CommandLineOptionOfBooleanType extends CommandLineOptionBase {
type: "boolean";
defaultValueDescription?: "true" | "false" | "undefined" | DiagnosticMessage;
Copy link
Contributor Author

@jablko jablko Oct 9, 2021

Choose a reason for hiding this comment

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

Also, what do you think about refining the CommandLineOption type like so, to check .type <-> .defaultValueDescription agreement?

orta
orta approved these changes Oct 11, 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.

Yeah, this seems reasonable to me - it doesn't look like a breaking change, but would be nice to have at least one other person take a look over it.

Switching from n/a is good simply because n/a is a tad ambiguous anyway.

@jablko
Copy link
Contributor Author

jablko commented Oct 11, 2021 •
edited
Loading

Thanks! I've now added a third commit to drop the optional modifier from CommandLineOptionOf{Number,Boolean}Type.defaultValueDescription. This checks for a defaultValueDescription of e.g. exactOptionalPropertyTypes, noFallthroughCasesInSwitch, noUncheckedIndexedAccess, noImplicitOverride, etc. (which I added in the same commit).

That adds default: false to the tsc --all output, like other options, e.g.
Screenshot from 2021-10-11 11-05-38

I added the commit to this PR because it builds on it, but I'm happy to move it to a separate PR if you prefer.

Here's the list of options I added defaultValueDescription: "false" to, to satisfy the refined CommandLineOptionOfBooleanType:

  • synchronousWatchDirectory
  • help
  • watch
  • preserveWatchOutput
  • explainFiles
  • assumeChangesOnlyAffectDirectDependencies
  • all
  • version
  • init
  • build
  • showConfig
  • listFilesOnly
  • useUnknownInCatchVariables
  • exactOptionalPropertyTypes
  • noFallthroughCasesInSwitch
  • noUncheckedIndexedAccess
  • noImplicitOverride
  • preserveSymlinks
  • experimentalDecorators
  • emitDecoratorMetadata
  • stripInternal
  • disableSourceOfProjectReferenceRedirect
  • disableSolutionSearching
  • disableReferencedProjectLoad
  • preserveConstEnums
  • preserveValueImports
  • verbose
  • dry
  • force
  • clean
  • enableAutoDiscovery
  • enable
  • disableFilenameBasedTypeAcquisition

@sandersn sandersn merged commit 248d478 into microsoft:main Oct 19, 2021
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@jablko @mprobst
Remove defaultValueDescription: "n/a" from options (microsoft#46280)
99be6cd 
* Remove defaultValueDescription: "n/a" from options

* CommandLineOption type <-> defaultValueDescription

* Require defaultValueDescription of number/boolean options
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn sandersn sandersn approved these changes

+1 more reviewer

@orta orta orta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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 @orta @sandersn