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.

Sign up for GitHub

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

Jump to bottom

Fix types: returnTransactionObjects param value affects resolve type #4911

Merged
merged 7 commits into from
May 4, 2022
Merged

Fix types: returnTransactionObjects param value affects resolve type #4911

merged 7 commits into from
May 4, 2022

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Apr 1, 2022 •
edited
Loading

Description

Documentation indicates that the value rather than the mere presence of the optional second parameter changes the type this call resolves to. This brings the typing in line with that.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas. [There aren't comments for similar code around it, and they don't seem necessary.]
  • I have made corresponding changes to the documentation. [This changes code to match the documentation.]
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules. [There are none.]
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code. [This command is broken and the response to Fix build command #4848 shows no real interest in getting that fixed.]
  • I ran npm run build with success.
  • I have tested dist/web3.min.js in a browser. [This only affects types, not transpiled code.]
  • I have tested my code on the live network. [This only affects types, not transpiled code.]
  • I have checked the Deploy Preview and it looks correct. [Still no idea how I'm supposed to do that.]
  • I have updated the CHANGELOG.md file in the root folder.

@wbt wbt changed the title returnTransactionObjects val affects resolve type returnTransactionObjects param value affects resolve type Apr 1, 2022
@wbt wbt changed the title returnTransactionObjects param value affects resolve type Fix types: returnTransactionObjects param value affects resolve type Apr 1, 2022
@coveralls
Copy link

coveralls commented Apr 1, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2079683730

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 429 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-2.3%) to 72.21%
Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 88.0%
packages/web3-core-helpers/src/formatters.js 25 81.07%
packages/web3-core-helpers/src/errors.js 31 4.41%
packages/web3-utils/src/soliditySha3.js 55 5.13%
packages/web3-utils/src/index.js 62 29.31%
packages/web3-utils/src/utils.js 92 12.86%
packages/web3-eth-accounts/src/index.js 163 23.77%
Totals Coverage Status
Change from base Build 2029198956: -2.3%
Covered Lines: 3368
Relevant Lines: 4396
💛 - Coveralls

@nazarhussain nazarhussain added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Apr 6, 2022
@wbt
Copy link
Contributor Author

wbt commented Apr 6, 2022 •
edited
Loading

Note that the one failing CI check is an error connecting to Infura which is failing other unrelated PRs as well; I don't believe it's caused by the changes in this PR. The Coveralls report that '429 unchanged lines in 7 files lost coverage' also seems erroneous and unrelated to this PR.

@wbt
Copy link
Contributor Author

wbt commented Apr 6, 2022

The new CI failure looks like the same one as here as a failure coming in from the base branch; a similar fix on the base branch might work.

@jdevcs jdevcs changed the base branch from 1.x to junaid/getblockTypesFixTests May 4, 2022 12:23
@jdevcs jdevcs merged commit ac45058 into web3:junaid/getblockTypesFixTests May 4, 2022
@jdevcs jdevcs mentioned this pull request May 4, 2022
Merged
16 tasks
jdevcs added a commit that referenced this pull request May 4, 2022
@jdevcs @wbt
Fix types: returnTransactionObjects param value affects resolve type (#…
7c4f1a5 
…4911) (#5000)

* returnTransactionObjects val affects resolve type

Co-authored-by: wbt <wbt@users.noreply.github.com>
@jdevcs
Copy link
Contributor

jdevcs commented May 4, 2022

This PR is merged via #5000

@wbt wbt deleted the enhance-getBlock-types branch May 4, 2022 16:27
@jdevcs jdevcs mentioned this pull request May 17, 2022
Merged
@Manny27nyc Manny27nyc mentioned this pull request Aug 10, 2022
Open
@PinkDiamond1 PinkDiamond1 mentioned this pull request Aug 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nazarhussain nazarhussain nazarhussain approved these changes

@nikoulai nikoulai nikoulai approved these changes

@luu-alex luu-alex luu-alex approved these changes

@jdevcs jdevcs jdevcs approved these changes

@spacesailor24 spacesailor24 Awaiting requested review from spacesailor24

Assignees
No one assigned
Labels
1.x 1.0 related issues Review Needed Maintainer(s) need to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wbt @coveralls @jdevcs @nazarhussain @nikoulai @luu-alex