Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

chore: remove lodash usage #6501

Merged
merged 10 commits into from
Mar 2, 2024
Merged

Conversation

HiroyukiNaito
Copy link
Contributor

Motivation

This is fix for the issue "Remove lodash usage #6489"

Description

Replace the lodash.pick for original pick function

Closes #6489

Steps to test or reproduce

yarn build
yarn test:unit

-->

@HiroyukiNaito HiroyukiNaito requested a review from a team as a code owner February 29, 2024 06:32
Copy link
Contributor

@jeluard jeluard left a comment

Choose a reason for hiding this comment

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

Looks like you need to run lint:fix, see output of GitHub checks.

packages/beacon-node/test/unit/eth1/utils/eth1Data.test.ts Outdated Show resolved Hide resolved
@HiroyukiNaito
Copy link
Contributor Author

Looks like you need to run lint:fix, see output of GitHub checks.

Sure, did it and pushed. Please review it if anytime you have.

@nflaig nflaig changed the title fix: remove lodash usage chore: remove lodash usage Feb 29, 2024
@HiroyukiNaito HiroyukiNaito requested a review from jeluard March 1, 2024 04:40
@jeluard
Copy link
Contributor

jeluard commented Mar 1, 2024

Please run yarn check-types too, one of the checks is failing.

@HiroyukiNaito
Copy link
Contributor Author

HiroyukiNaito commented Mar 1, 2024

Please run yarn check-types too, one of the checks is failing.

@lodestar/beacon-node: $ tsc
@lodestar/beacon-node: test/unit/eth1/utils/eth1Data.test.ts:109:77 - error TS2345: Argument of type 'string[]' is not assignable to parameter of type '("depositRoot" | "depositCount" | keyof Eth1Block)[]'.
@lodestar/beacon-node:   Type 'string' is not assignable to type '"depositRoot" | "depositCount" | keyof Eth1Block'.
@lodestar/beacon-node: 109         const eth1DatasPartial = eth1Datas.map((eth1Data) => pick(eth1Data, Object.keys(expectedEth1Data[0])));
@lodestar/beacon-node:                                                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@lodestar/beacon-node: Found 1 error in test/unit/eth1/utils/eth1Data.test.ts:109

@HiroyukiNaito
Copy link
Contributor Author

Please run yarn check-types too, one of the checks is failing.

According to the Error. Changed string[] assignable in args.

@HiroyukiNaito HiroyukiNaito requested a review from jeluard March 1, 2024 17:41
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Merging #6501 (59b6942) into unstable (1f18ec4) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #6501   +/-   ##
=========================================
  Coverage     61.72%   61.72%           
=========================================
  Files           555      555           
  Lines         58206    58206           
  Branches       1844     1844           
=========================================
  Hits          35929    35929           
  Misses        22238    22238           
  Partials         39       39           

@@ -274,3 +273,15 @@ function getMockDeposit({blockNumber, index}: {blockNumber: number; index: numbe
depositData: {} as phase0.DepositData, // Not used
};
}

function pick<T, K extends keyof T>(obj: T, keys: string[]): Pick<T, K> {
return (keys as K[]).reduce(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this cast?
Also I am skeptical we need this Pick use here.

A different path would be to replca the pick usage altogether and replace it with something like:

...
const eth1DatasPartial = eth1Datas.map(({blockNumber, depositCount}) => ({blockNumber, depositCount}));
...

Sorry for the back and forth!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.
That's must be the answer for the simplest refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your insights have really helped me improve my code. Thank you for your review.

@HiroyukiNaito HiroyukiNaito requested a review from jeluard March 2, 2024 03:39
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

LGTM

@jeluard jeluard merged commit b5712a6 into ChainSafe:unstable Mar 2, 2024
20 checks passed
@jeluard
Copy link
Contributor

jeluard commented Mar 2, 2024

Thanks @HiroyukiNaito for the effort!

@HiroyukiNaito
Copy link
Contributor Author

Thanks for the dedicated support! I return the favor in the another issue.

@wemeetagain
Copy link
Member

🎉 This PR is included in v1.17.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove lodash usage
4 participants