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

Add removed property to Log type #4877

Merged
merged 5 commits into from Oct 28, 2022

Conversation

samlaf
Copy link

@samlaf samlaf commented Mar 21, 2022

Attempt at Fixing #4747 (comment)

This is just trying to move forward in fixing this issue. I don't have the time to deep dive into web3js and try to understand how to make sure that this PR is non-breaking, so would like for someone else to help me verify.

I just added the property and ran tsc --noEmit in packages/web3-core which type checked.
However I am not sure if it breaks some of the other web3 packages, since when running tsc --noEmit in the root folder, I get a gazillion type errors from the tests, such as

packages/web3-utils/types/tests/string-to-hex-test.ts:40:13 - error TS2345: Argument of type 'boolean' is not assignable to parameter of type 'string'.

40 stringToHex(true);

which comes from

// $ExpectError
stringToHex(true);

Perhaps we could add // @ts-ignore above these lines, but I'm not sure if it wouldn't break the // $ExpectError commands (maybe only the line before the actual test applies?).

@jdevcs jdevcs added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Mar 30, 2022
added this upon jdevcs' [request](web3#4877 (comment))
Copy link
Contributor

@avkos avkos left a comment

Choose a reason for hiding this comment

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

@samlaf Please resolve conflicts

Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts. Rest LGTM.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2161428870

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 72.217%

Totals Coverage Status
Change from base Build 2155746686: 0.0%
Covered Lines: 3372
Relevant Lines: 4401

💛 - Coveralls

@jdevcs jdevcs requested a review from avkos May 4, 2022 10:37
@github-actions
Copy link

github-actions bot commented Jul 4, 2022

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Jul 4, 2022
@jdevcs jdevcs removed the Stale Has not received enough activity label Jul 6, 2022
@jdevcs jdevcs added Future Release For future release. and removed Review Needed Maintainer(s) need to review Future Release For future release. labels Sep 6, 2022
@jdevcs jdevcs changed the base branch from 1.x to junaid/removedpropertytologtypee2etest October 28, 2022 10:39
@jdevcs jdevcs merged commit 134c923 into web3:junaid/removedpropertytologtypee2etest Oct 28, 2022
@jdevcs
Copy link
Contributor

jdevcs commented Oct 28, 2022

Merged into 1.x via #5576

jdevcs added a commit that referenced this pull request Oct 28, 2022
* Add `removed` property to `Log` type

Fixes #4747 (comment)

* Update CHANGELOG.md

added this upon jdevcs' [request](#4877 (comment))

* Update CHANGELOG.md

Co-authored-by: Junaid <86780488+jdevcs@users.noreply.github.com>

Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
@jdevcs jdevcs mentioned this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants