ENH: query price presentation (uses pyfiglet fonts)#71
Conversation
Codecov Report
@@ Coverage Diff @@
## master #71 +/- ##
===========================================
- Coverage 73.99% 28.17% -45.83%
===========================================
Files 4 4
Lines 1542 1544 +2
===========================================
- Hits 1141 435 -706
- Misses 401 1109 +708
Continue to review full report at Codecov.
|
| import sys | ||
|
|
||
| import numpy as np | ||
| from pyfiglet import Figlet |
There was a problem hiding this comment.
This would need to be added as a dependency in setup.py as well.
|
|
||
| def query_price_for(self, bytes_num): | ||
| figlet = None | ||
| price = bytes_num * self.query_price_for_TB / 2**40 |
There was a problem hiding this comment.
I think it's reasonable to put the price estimate in, but note that it won't always apply since there is also a fixed price plan.
I'm not sure about the fancy text rendering. It's fun, but is it worth the extra dependency?
There was a problem hiding this comment.
Unfortunately, I agree re the extra dependency. Maaaybe as a soft dependency
There was a problem hiding this comment.
My personal preference is to remove the pyfiglet font and just report the pricing in the default font.
There was a problem hiding this comment.
ok, so if there is no hard veto's (@jreback?!) i'll try to push this forward without figlet fonts
There was a problem hiding this comment.
thus shouldn't be a hard dep
There was a problem hiding this comment.
i can make it an option i guess but not really sure it adds value
|
|
||
| def query_price_for(self, bytes_num): | ||
| figlet = None | ||
| price = bytes_num * self.query_price_for_TB / 2**40 |
There was a problem hiding this comment.
My personal preference is to remove the pyfiglet font and just report the pricing in the default font.
| num /= 1024.0 | ||
| return fmt % (num, 'Y', suffix) | ||
|
|
||
| def query_price_for(self, bytes_num): |
There was a problem hiding this comment.
Could we rename query_price_for to something more descriptive like query_price_from_byte_count or similar?
There was a problem hiding this comment.
Could you also add a unit test for query_price_for so that our code coverage doesn't decrease?
8ee1efd to
fd5e0e6
Compare
|
now it's clean and easy! please revie |
|
@parthea, good point, thx |
|
Sample output is: |
|
generally wait for comments on PRs before merging pls |
|
Generally, ok. But in this specific case, there were two approval comments, and since my further changes does only modified string literal, which pass my local manual checks and Travis builds as well, I've decided to go forward and merge them. Is there any problem with merged code? |
|
@tworec not a problem. |
|
ok :) |
Hi,
to keep up the momentum I'm sharing my fancy query price print outs.
Feel free to abandon this if you do not like it.
At RTBHOUSE we are using it with pleasure :)
Feature:
Query price is printed with ascii art font. Font gets larger as price increases.
Sample outputs