Skip to content

Conversation

@zlumer
Copy link
Contributor

@zlumer zlumer commented Aug 6, 2018

Fixes #27851

  • Use a meaningful title for the pull request. Include the name of the package modified.

  • Test the change in your own code. (Compile and run.)

  • Add or edit tests to reflect the change. (Run with npm test.)

  • Follow the advice from the readme.

  • Avoid common mistakes.

  • Run npm run lint package-name (or tsc if no tslint.json is present).

  • Provide a URL to documentation or source code which provides context for the suggested changes: http://web3js.readthedocs.io/en/1.0/web3.html#utils

@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 6, 2018

@zlumer Thank you for submitting this PR!

🔔 @simon-jentzsch @nitzantomer @zurbo @yxliang01 @phra @naddison36 @icaroharry @linusnorton @jpeletier @anneau @matrushka @andrevmatos @levino - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@archangel-irk
Copy link
Contributor

archangel-irk commented Aug 8, 2018

I think the current state of PR is not fixing the issue, because of the 'utils' is a static property of Web3 class.

It could be resolved like this:

declare class Web3 {
  ...
  static utils: Utils;
  ...
}

Copy link
Contributor

@levino levino left a comment

Choose a reason for hiding this comment

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

This PR would introduce a bug instead of fixing the issue. Maybe the issue should be made more precise for collaborators to understand the problem. #27851


const weiStr: string = web3.utils.toWei("100", "gwei");
const weiBn: BigNumber = web3.utils.toWei(web3.utils.toBN("1"));
const rndHex: string = Web3.randomHex(10);
Copy link
Contributor

Choose a reason for hiding this comment

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

Web3 does not have a method randomHex. Web3.utils has.

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Aug 8, 2018
@typescript-bot
Copy link
Contributor

@zlumer One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 8, 2018
@zlumer
Copy link
Contributor Author

zlumer commented Aug 8, 2018

@levino good catch, fixed.

@zlumer
Copy link
Contributor Author

zlumer commented Aug 8, 2018

See also #27963

@levino
Copy link
Contributor

levino commented Aug 8, 2018

@zlumer @archangel-irk was faster than you. Lets close your PR in favour of #27963

Remark: If we would accept your PR we would have f42b3db forever in the history (including a bug). If anyone would be stupid enough to cherry pick, they would have a problem. It is better to remove such footguns from the history by rebasing and squashing the commits into one and then force push over your branch here. This is just a remark for future PRs.

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 8, 2018
@typescript-bot
Copy link
Contributor

🔔 @levino - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 8, 2018
@zlumer
Copy link
Contributor Author

zlumer commented Aug 8, 2018

@levino agree, closing.

@zlumer zlumer closed this Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Revision needed This PR needs code changes before it can be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants