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 JSDoc to web3-utils #331

Merged
merged 9 commits into from Oct 18, 2018

Conversation

Projects
None yet
3 participants
@juslar
Copy link
Contributor

juslar commented Aug 22, 2018

Fix #307

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Aug 22, 2018

CLA assistant check
All committers have signed the CLA.

@sohkai
Copy link
Member

sohkai left a comment

❤️ thanks for the contribution! Really liking it so far :).

Just a couple of nitpicky comments.

// Check address equality without checksums
/** Check address equality without checksums
* @param {string} first - First address
* @param {string} second - Second address

This comment has been minimized.

@sohkai

sohkai Aug 23, 2018

Member

🙏 thanks for adding JSDoc comments! It'd be great if these also had their returns documented :).

//
/**
* Takes a Wei balance and returns it rounded to the number of digits after the decimal point specified (by default 2 digits).
* @param {string} balance - Balance in Wei

This comment has been minimized.

@sohkai

sohkai Aug 23, 2018

Member

I think the comments here are misleading, since it returns a converted (e.g. to 'ether' digits) value. I'd also just name the first argument wei, since it doesn't have to be an account balance.

* @param {string} balance - Balance in Wei
* @param {number} [digits=2] - Number of digits after the decimal point
*/
export function fromWeiRounded(balance, digits = 2) {

This comment has been minimized.

@sohkai

sohkai Aug 23, 2018

Member

What do you think about adding an optional param for the unit to convert to, to pass into fromWei()?

export function fromWeiRounded(balance, digits = 2) {
var ethBalance = Web3.utils.fromWei(balance, 'ether')
var decimalIndex = ethBalance.indexOf('.') + 1
return ethBalance.substring(0, decimalIndex != -1 ? decimalIndex + digits : ethBalance.length)

This comment has been minimized.

@sohkai

sohkai Aug 23, 2018

Member

This should actually handle the rounding at decimalIndex + digits, e.g. this would turn 12.345 into 12.34 rather than 12.35.

@juslar

This comment has been minimized.

Copy link
Contributor

juslar commented Aug 23, 2018

Thank you so much for the great feedback, I really appreciate it! 😊

Sorry for the silly mistake on my part! I made the requested changes. Let me know if it's ok!

@@ -4,22 +4,46 @@
* from this file.
*/
import Web3 from 'web3'
import BigNumber from 'bignumber.js'

This comment has been minimized.

@sohkai

sohkai Aug 29, 2018

Member

Unfortunately we're avoiding bignumber.js to avoid bundling two BN implementations (BN.js is faster and lighter, but doesn't support the nice rounding that comes with bignumber.js ;).

@@ -225,7 +225,7 @@ class StartContent extends React.PureComponent {
return (
<ActionInfo>
You need at least {fromWei(MINIMUM_BALANCE)} ETH (you have{' '}
{Math.round(parseInt(fromWei(balance || '0'), 10) * 1000) / 1000}{' '}
{fromWeiRounded(balance)}{' '}

This comment has been minimized.

@sohkai

sohkai Aug 29, 2018

Member

I think we still need the balance || '0' in case balance is null.

@sohkai sohkai self-assigned this Aug 30, 2018

@juslar

This comment has been minimized.

Copy link
Contributor

juslar commented Sep 4, 2018

Ok I understand. I changed back fromWeiRounded() to a string splitting method but this time it rounds it up like it's supposed to! Thanks again :)

@sohkai
Copy link
Member

sohkai left a comment

Thanks @juslar! A few more comments:

* shortenAddress('0x197319') // 0x197319 (already short enough)
*
* @param {string} address - The address to shorten
* @param {number}[charsLength=4] - The number of characters to change on both sides of the ellipsis

This comment has been minimized.

@sohkai

sohkai Sep 11, 2018

Member

I think there should be a space between the } and [

export function fromWeiRounded(wei, digits = 2, unit = 'ether') {
let unitValue = Web3.utils.fromWei(wei, unit)
const decimalIndex = unitValue.indexOf('.')
if(decimalIndex != -1) {

This comment has been minimized.

@sohkai

sohkai Sep 11, 2018

Member

There's a lot of places where the linter should be failing—could you run npm run lint (it'll now automatically do this if merge from master and try to push).

if(decimalIndex != -1) {
unitValue = unitValue.substring(0, decimalIndex + digits + 2)
const roundedLastDigit = unitValue[unitValue.length-2].concat('.' + unitValue[unitValue.length -1])
unitValue = unitValue.substring(0, unitValue.length - 2).concat(Math.round(roundedLastDigit))

This comment has been minimized.

@sohkai

sohkai Sep 11, 2018

Member

I think this is a little bit overkill for what we're trying to achieve.

We could simply get the last digit that we care about, and do a lastDigit >= 5 ? lastDigit + 1 : lastDigit calculation instead.

@sohkai sohkai referenced this pull request Oct 13, 2018

Merged

Add `formatNumber()` util #392

sohkai added some commits Oct 18, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 18, 2018

@juslar We've superceded this PR with #392 for now, so I've modified this PR to be about adding the relevant JSDoc instead.

Thanks for adding them and trying to get this implemented!

@sohkai sohkai changed the title Implement fromWeiRounded() Add JSDoc to web3-utils Oct 18, 2018

@sohkai sohkai merged commit 60a648c into aragon:master Oct 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment