Skip to content
This repository was archived by the owner on Aug 22, 2023. It is now read-only.

Add better type for flags with default value#53

Merged
RasPhilCo merged 4 commits into
oclif:masterfrom
frangio:default-no-undefined
Jul 11, 2019
Merged

Add better type for flags with default value#53
RasPhilCo merged 4 commits into
oclif:masterfrom
frangio:default-no-undefined

Conversation

@frangio
Copy link
Copy Markdown
Contributor

@frangio frangio commented Jun 27, 2019

A flag with a default value was being typed as as T | undefined. For example the following would not type:

      const out = parse([], {
        flags: {
          foo: flags.string({ default: 'baz' }),
          bar: flags.integer({ default: 42 }),
        }
      })
      const foo: string = out.flags.foo;
      const bar: number = out.flags.bar;

I added a special case in the type for this, the same way that required: true works.

Making the flag required is a workaround to get the above to type, but this is undesirable because it incorrectly shows the flag as required in the help.

@salesforce-cla
Copy link
Copy Markdown

Thanks for the contribution! Before we can merge this, we need @frangio to sign the Salesforce.com Contributor License Agreement.

@frangio frangio force-pushed the default-no-undefined branch from b009898 to 9189c22 Compare June 27, 2019 04:06
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 27, 2019

Codecov Report

Merging #53 into master will decrease coverage by 91.35%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #53       +/-   ##
========================================
- Coverage   91.35%    0%   -91.36%     
========================================
  Files          11     1       -10     
  Lines         324    29      -295     
  Branches       85    14       -71     
========================================
- Hits          296     0      -296     
- Misses         12    29       +17     
+ Partials       16     0       -16
Impacted Files Coverage Δ
src/validate.ts 0% <0%> (-96.88%) ⬇️
src/deps.ts
src/index.ts
src/help.ts
src/util.ts
src/list.ts
src/screen.ts
src/args.ts
src/errors.ts
src/parse.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3880ae...9add94f. Read the comment docs.

@frangio frangio force-pushed the default-no-undefined branch from 420b523 to 41c6aec Compare June 29, 2019 22:39
@frangio
Copy link
Copy Markdown
Contributor Author

frangio commented Jul 1, 2019

Not sure why the CI is not running. A previous run had passed.

@frangio
Copy link
Copy Markdown
Contributor Author

frangio commented Jul 9, 2019

@RasPhilCo any chance to get this small PR merged? 🙏

Thanks!

@RasPhilCo
Copy link
Copy Markdown
Contributor

RasPhilCo commented Jul 10, 2019

Nice! Looking into why Circle stopped running... Circle is back.

@frangio
Copy link
Copy Markdown
Contributor Author

frangio commented Jul 10, 2019

The CI failure seems unrelated to this PR. master is failing for the same reason. What do you suggest? I can look into fixing it. The problem is an implicit any type.

@RasPhilCo
Copy link
Copy Markdown
Contributor

Can you push another commit with a comment or something small to trigger CI again? I had to re-add the repo and I think there's a caching issue.

@frangio
Copy link
Copy Markdown
Contributor Author

frangio commented Jul 11, 2019

I fixed what was causing the tests to fail.

This coverage report is very odd-looking though. 🤔 -91.36%?

Copy link
Copy Markdown
Contributor

@RasPhilCo RasPhilCo left a comment

Choose a reason for hiding this comment

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

🏄

@RasPhilCo RasPhilCo merged commit 044806a into oclif:master Jul 11, 2019
@frangio frangio deleted the default-no-undefined branch July 11, 2019 23:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants