Skip to content

Conversation

@agubler
Copy link
Member

@agubler agubler commented Mar 8, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Convert to use @types/yargs.

Discussion as we do lose the typings for Argv that seem to be typed as any in the latest yarg types from @types.

Resolves #119

@codecov-io
Copy link

codecov-io commented Mar 8, 2017

Codecov Report

Merging #120 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   99.45%   99.45%           
=======================================
  Files          15       15           
  Lines         369      369           
  Branches       42       42           
=======================================
  Hits          367      367           
  Partials        2        2
Impacted Files Coverage Δ
src/Helper.ts 100% <ø> (ø) ⬆️
src/CommandHelper.ts 100% <ø> (ø) ⬆️
src/commands/eject.ts 100% <ø> (ø) ⬆️
src/registerCommands.ts 100% <100%> (ø) ⬆️

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 153c571...7f5cb81. Read the comment docs.

@dylans dylans added this to the 2017.03 milestone Mar 8, 2017
private context: any;
private configuration: ConfigurationHelper;
run(group: string, commandName?: string, args?: yargs.Argv): Promise<any> {
run(group: string, commandName?: string, args?: any): Promise<any> {
Copy link
Member

Choose a reason for hiding this comment

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

can't this become Argv ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because what was Yargs is now Argv and the typing for what was Argv is now any.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did mention this in the description.

Copy link
Member

Choose a reason for hiding this comment

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

I see that now.

@dylans dylans modified the milestones: 2017.04, 2017.03 Mar 27, 2017
@dylans
Copy link
Member

dylans commented Apr 29, 2017

@agubler any reason to not land this after a rebase?

@agubler
Copy link
Member Author

agubler commented May 2, 2017

@dylans just the fact that the typings don't seem to be quite as good. Also it's a breaking change as we'd need to convert all the commands.

@dylans dylans modified the milestones: 2017.05, 2017.04 May 2, 2017
@eheasley eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017
@dylans dylans modified the milestones: 2017.06, 2017.07 Jul 4, 2017
@dylans dylans modified the milestones: 2017.08, 2017.07 Jul 29, 2017
@agubler agubler closed this Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants