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

types: fixed utils.fromWei() signature to match docs #2214

Closed
wants to merge 1 commit into from

Conversation

zlumer
Copy link

@zlumer zlumer commented Jan 26, 2019

Description

Migrated code from DefinitelyTyped/DefinitelyTyped#32409 by @alexkvak

Type of change

  • Bug fix

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.
  • I have made corresponding changes to the documentation.
  • My changes generate no warnings.
  • I have updated or added types for all modules I've changed
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test with success and extended the tests if necessary.
  • I ran npm run build and tested it in the browser and with node.
  • I ran npm run-script dtslint and tested that all my types are correct
  • I have tested my code on the live network.

@nivida nivida added the Types Incorrect or missing types label Jan 26, 2019
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.704% when pulling e84b7fa on zlumer:fix/fromWei into 5db55c2 on ethereum:1.0.

@joshstevens19
Copy link
Contributor

joshstevens19 commented Jan 26, 2019

Hey thanks for this dude, if you could update the method docs as well that would be great

web3-utils line 186 and line 189

 * @method fromWei
 *
 * @param {Number|String} number can be a number, number string or a HEX of a decimal
 * @param {String} unit the unit to convert to, default ether
 *
 * @returns {String|Object} When given a BN object it returns one as well, otherwise a number
 */
export const fromWei = (number, unit) => {

this way we can update the internal code docs with the external docs and typings. I will then approve.

@zlumer
Copy link
Author

zlumer commented Jan 26, 2019

I just noticed that there are two different fromWei() definitions:
https://github.com/ethereum/web3.js/blob/e84b7fa2aff8a9dbc5b9b611aa1fd5df0ce0ec4c/packages/web3-utils/types/index.d.ts#L100
https://github.com/ethereum/web3.js/blob/e84b7fa2aff8a9dbc5b9b611aa1fd5df0ce0ec4c/packages/web3-utils/types/index.d.ts#L149
This PR is therefore incorrect. The same applies to #2215
Only one set of the functions is covered with tests.
Some refactoring is needed to improve code quality and remove duplication before such PRs could be merged effectively.

@joshstevens19
Copy link
Contributor

Let me take a look at why this was done like this and get back you tomorrow 👍thanks

@joshstevens19
Copy link
Contributor

Hey,

I have put the fixes in #2218 for the ones you mentioned in DefinitelyTyped/DefinitelyTyped#32493

The reason we have a interface is its used in other typing's in different packages like:

https://github.com/ethereum/web3.js/blob/1.0/packages/web3-core-method/types/index.d.ts#L27
https://github.com/ethereum/web3.js/blob/1.0/packages/web3-eth-ens/types/index.d.ts#L76
https://github.com/ethereum/web3.js/blob/1.0/packages/web3/types/index.d.ts#L39

So obviously we needed a interface to define the type of for example web3.Utils but we also wanted to introduce ability to only import what you need within the Utils lib i.e.

import { asciiToHex } from 'web3-utils';

this was the most cleanest way to do this so developers can use partial imports in their code as well as a nice interface on the Utils so other packages can reuse it. This is the only place it repeats typing's like this.

You are correct it seems we don't have any tests for that interface which i will add to my checklist (just double checked and this is the only place not tested, I will jump on that when I have a second).

So yeah at least this will be fixed on next release. If you do spot anything else or any other PR which went into DT web3 let me know or do a PR and we get it merged in.

Cheers

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

Successfully merging this pull request may close these issues.

4 participants