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

Util: treeshaking tasks #3589

Merged
merged 16 commits into from
Aug 29, 2024
Merged

Util: treeshaking tasks #3589

merged 16 commits into from
Aug 29, 2024

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Aug 14, 2024

Util treeshaking task cleanup from #3560

  • withdrawal.ts
    • Withdrawal static constructors extracted to functions
  • requests.ts renamed request.ts
    • static constructors extracted to functions
      • WithdrawalRequest
      • ConsolidationRequest
      • DepositRequest

@holgerd77
Copy link
Member

In-between feedback: this looks good so far

@ScottyPoi
Copy link
Contributor Author

In-between feedback: this looks good so far

Ran out of time before the weekend. Will finish this up today

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 71.05263% with 33 lines in your changes missing coverage. Please review.

Project coverage is 20.69%. Comparing base (4470cc3) to head (2c368ea).
Report is 21 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain 84.92% <ø> (?)
client ?
common 89.68% <ø> (?)
devp2p 0.00% <ø> (?)
genesis 0.00% <ø> (?)
tx ?
util 71.45% <71.05%> (?)
wallet ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could disabling no-use-before-define be avoided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here, could disabling no-use-before-define be avoided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Some basic structural things to discuss respectively adjust.

packages/block/test/eip4895block.spec.ts Outdated Show resolved Hide resolved
packages/util/src/request.ts Show resolved Hide resolved
@scorbajio
Copy link
Contributor

Please update the example embeddings in the util docs and any other package docs that have modified examples from this PR. Also see here for more details.

@holgerd77
Copy link
Member

Can this be finalized so that we can merge?

@ScottyPoi
Copy link
Contributor Author

changes addressed. ready for re-review

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@acolytec3 acolytec3 dismissed holgerd77’s stale review August 29, 2024 18:59

Has been addressed

@acolytec3 acolytec3 merged commit dc906d0 into master Aug 29, 2024
38 checks passed
@scorbajio scorbajio deleted the util-treeshake branch August 30, 2024 17:15
@holgerd77
Copy link
Member

Nice, looks good! 🙂

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.

4 participants