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

Add better type for flags with default value #53

Merged
merged 4 commits into from
Jul 11, 2019

Conversation

frangio
Copy link
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

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

@codecov
Copy link

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
Copy link
Contributor Author

frangio commented Jul 1, 2019

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

@frangio
Copy link
Contributor Author

frangio commented Jul 9, 2019

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

Thanks!

@RasPhilCo
Copy link
Contributor

RasPhilCo commented Jul 10, 2019

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

@frangio
Copy link
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
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
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
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.

None yet

2 participants