Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: improve TypeScript types #650

Merged
merged 2 commits into from
Mar 19, 2020
Merged

fix: improve TypeScript types #650

merged 2 commits into from
Mar 19, 2020

Conversation

JustinBeckwith
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 19, 2020
alexander-fenster
alexander-fenster reviewed Mar 19, 2020
View reviewed changes
src/chunktransformer.ts
const row = this.row!;
row.key = newRowKey;
row.data = {};
this.family = row.data[chunk.familyName.value] = {} as Family;
Copy link
Contributor

Choose a reason for hiding this comment

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

am I dreaming, or am I really seeing the good old a = b = {} in the real-life JavaScript code? o_O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying not to change the functionality here, but yeah....

Copy link
Contributor

Choose a reason for hiding this comment

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

You know what, I'm taking this back! I just realized that it might be what they actually wanted to assign the same object to both this.family and row.data[chunk.familyName.value]. I'm afraid of this code :(

alexander-fenster
alexander-fenster suggested changes Mar 19, 2020
View reviewed changes
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.

Possibly unintended logic change related to RegExp

@codecov
Copy link

codecov bot commented Mar 19, 2020 •
edited
Loading

Codecov Report

Merging #650 into master will increase coverage by 0.03%.
The diff coverage is 99.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   96.35%   96.38%   +0.03%     
==========================================
  Files          15       15              
  Lines       12993    13118     +125     
  Branches      809      810       +1     
==========================================
+ Hits        12519    12644     +125     
  Misses        471      471              
  Partials        3        3
Impacted Files Coverage Δ
src/table.ts 99.83% <85.71%> (-0.01%) ⬇️
src/chunktransformer.ts 100.00% <100.00%> (ø)
src/filter.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/instance.ts 99.90% <100.00%> (+<0.01%) ⬆️

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 645dff9...c832981. Read the comment docs.

@JustinBeckwith
Copy link
Contributor Author

@alexander-fenster ready for another look (this time with tests passing!)

@JustinBeckwith JustinBeckwith merged commit b54830f into master Mar 19, 2020
@laljikanjareeya laljikanjareeya mentioned this pull request Mar 23, 2020
Closed
@stephenplusplus stephenplusplus deleted the types5 branch August 10, 2020 17:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@alexander-fenster alexander-fenster alexander-fenster approved these changes

@bcoe bcoe Awaiting requested review from bcoe

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JustinBeckwith @alexander-fenster @googlebot