Skip to content

fix: improve typescript types for Table class#641

Merged
JustinBeckwith merged 2 commits intomasterfrom
table-types
Mar 17, 2020
Merged

fix: improve typescript types for Table class#641
JustinBeckwith merged 2 commits intomasterfrom
table-types

Conversation

@JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Mar 15, 2020

Incrementally inching towards #635. This ensures proper types in table.ts.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 15, 2020
@JustinBeckwith JustinBeckwith requested a review from bcoe March 15, 2020 18:26
@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #641 into master will increase coverage by 0.09%.
The diff coverage is 99.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   96.22%   96.31%   +0.09%     
==========================================
  Files          15       15              
  Lines       12499    12819     +320     
  Branches      789      799      +10     
==========================================
+ Hits        12027    12347     +320     
  Misses        469      469              
  Partials        3        3              
Impacted Files Coverage Δ
src/table.ts 99.83% <99.40%> (+0.03%) ⬆️
src/family.ts 99.58% <100.00%> (-0.01%) ⬇️
src/index.ts 100.00% <100.00%> (ø)

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 87c2c6b...50dc86e. Read the comment docs.

@JustinBeckwith JustinBeckwith requested a review from kolea2 March 15, 2020 18:27

let Table: typeof tblTypes.Table;
let table: tblTypes.Table;
let table: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need eslint-ignore here? how does it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, good call out. For some reason we don't... will fix this after we have user land types working properly (more important that tests)

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

LGTM - but we'll need to do more work here getting rid of proto/ folder.

@JustinBeckwith JustinBeckwith merged commit 68179d1 into master Mar 17, 2020
Copy link

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@kolea2
Copy link
Contributor

kolea2 commented Mar 18, 2020

LGTM thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants