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

Make disconnect params optional in types #4833

Merged
merged 7 commits into from
Mar 23, 2022

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Mar 8, 2022

Description

Though the disconnect function isn't documented, in practice it does not seem to require two params, and the main example given omits them. The second is absent in multiple tests. Both are absent in other tests.

The implementation which does seem to want it can be found here but the first parameter has an effective default value of 1000 and the second is only passed on to this.connection.close() which is implemented here. That implementation only passes it on to this._connection.close() and as seen by parameterless calls to the same function two lines below each two-parameter call, those are optional.

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. [This does not seem necessary for this simple 2-character change.]
  • I have made corresponding changes to the documentation. [The function isn't documented so no changes are needed; this brings the typing more in line with documentation that already does exist.]
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules. [There aren't any.]
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success. [Noting that the tests are unreliable on the base branch.]
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code. [This command's use of semicolons instead of ampersands mean this superset of the line above has been broken for years. Fixing it here seems unnecessary: Existing test cases cover the change, or would if they were TypeScript, but changing that is beyond the scope of this PR.]
  • I ran npm run build with success.
  • I have tested dist/web3.min.js in a browser. [This doesn't make changes in compiled code.]
  • I have tested my code on the live network. [This doesn't make changes in compiled code.]
  • I have checked the Deploy Preview and it looks correct. [I'm not sure what this refers to or how to do this; further instructions could be helpful.]
  • I have updated the CHANGELOG.md file in the root folder.

@coveralls
Copy link

coveralls commented Mar 8, 2022

Pull Request Test Coverage Report for Build 2017514570

  • 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 2002669572: -2.3%
Covered Lines: 3368
Relevant Lines: 4396

💛 - Coveralls

@wbt
Copy link
Contributor Author

wbt commented Mar 8, 2022

The failure is 'Failed to connect to Infura over websockets after 10 tries' which I don't think is a consequence of this very minor PR; it seems more likely to be an issue at Infura related to some connectivity issues they've been having lately. Also, I'm not sure if the IP address being used for the test is in the range of newly censored IP addresses intentionally blocked from connecting to Infura, or one of those adjacent to it in some way that leads it to get caught up in the block.

The post about test code coverage changing is also inaccurate. This PR shouldn't change test coverage, and doesn't even touch the files cited as having new missed lines.

mostly just to kick off CI again.
@wbt
Copy link
Contributor Author

wbt commented Mar 8, 2022

Thought it is an improvement worth merging in, the extra change was intended just to kick off the CI again because I don't have permission to do that directly, and note that the tests are unreliable when run locally. It produced the same issue, which I think is an Infura or testing infrastructure issue rather than a bug newly added in the branch proposed for merging.

@nazarhussain nazarhussain added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Mar 9, 2022
@jdevcs jdevcs changed the base branch from 1.x to junaid/optionaldcparam4833 March 23, 2022 12:19
@jdevcs jdevcs merged commit 4376c1e into web3:junaid/optionaldcparam4833 Mar 23, 2022
jdevcs added a commit that referenced this pull request Mar 23, 2022
* Make disconnect params optional in types

* Update changelog, including PR number

* Update refs to old discussions,
mostly just to kick off CI again.

* PR under 1.7.2

Co-authored-by: wbt <[email protected]>
@wbt wbt deleted the patch-4 branch March 23, 2022 16:00
@jdevcs jdevcs mentioned this pull request Mar 23, 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 Review Needed Maintainer(s) need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants