Address all Leandro's feedback
- Dominant language
- Python
- Stars
- 5
- Forks
- 3
- PR merge metrics
- No merged PRs in 30d
Description
I'll accumulate in this issue, all the good tips from Leandro.
Since there's MANY waterfall PRs and approved with nits and comments, I'll make this issue so I merge all and then address the suggested comments :)
# 1. Use var for price:
* [x] Done?
Nit: I would put the prices into their own variables and simplify the string building to something like:
```js
price = format_price(
calculate_price(
numerator=boughtVolume,
denominator=soldVolume,
decimals_numerator=buyTokenDecimals,
decimals_denominator=sellTokenDecimals
),
currency=buyTokenLabel
)
priceStr1 = f'{click.style(f' Avg. Traded Price {sellTokenLabel}/{buyTokenLabel}', fg=labelColor)}: {price1}'
...
tradePriceText = f'{priceStr1}\n{priceStr2}\n'
```
https://github.com/gnosis/dex-cli/pull/32#discussion_r394413888
# 2. Max sell volume, max receiving amount
* [ ] Done?
Would it be too much to also add the order size? Meaning, max buy/sell amounts, not just the matched amounts.
https://github.com/gnosis/dex-cli/pull/32#discussion_r394415191
# 3. Use f strings for formatting numbers
* [x] Done?
def format_integer(number):
return str(number) # TODO: Format better the numbers
return '{:,d}'.format(number)
Also in:
```
return f'{{:,.{decimals}f}}'.format(rounded_value).rstrip('0').rstrip('.')
```
Also in:
`return format_amount(percentage, decimals=2) + '%'`
https://github.com/gnosis/dex-cli/pull/34#discussion_r394467039
# 4. Unreachable code in `calculate_price`
* [x] Done?
https://github.com/gnosis/dex-cli/pull/32#discussion_r394425752
# 5 Simplify expression
* [x] Done?
https://github.com/gnosis/dex-cli/pull/32#discussion_r394421727
# 6. Improve sorting
* [ ] Done?
Can you enforce the option only accepts asc or desc?
What happens if something else is input?
https://github.com/gnosis/dex-cli/pull/33#discussion_r394462812
# 7. Use snake case for vars
* [x] Done?
Not comment from leandro, but I saw it now :)
`isUnlimitedAmount`
# 8. Improve for comprehension
Python magic 🌟
* [x] Done? This:
https://github.com/gnosis/dex-cli/pull/37#discussion_r394500885
* [x] Done? And this:
https://github.com/gnosis/dex-cli/pull/37#discussion_r394502145
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.