-
Notifications
You must be signed in to change notification settings - Fork 125
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
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.
|
pandas_gbq/gbq.py
Outdated
@@ -7,6 +7,9 @@ | |||
import sys | |||
|
|||
import numpy as np | |||
from pyfiglet import Figlet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need to be added as a dependency in setup.py
as well.
pandas_gbq/gbq.py
Outdated
@@ -421,6 +439,17 @@ def sizeof_fmt(num, suffix='B'): | |||
num /= 1024.0 | |||
return fmt % (num, 'Y', suffix) | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I agree re the extra dependency. Maaaybe as a soft dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thus shouldn't be a hard dep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can make it an option i guess but not really sure it adds value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i -> you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tworec ! I have a couple minor observations.
pandas_gbq/gbq.py
Outdated
@@ -421,6 +439,17 @@ def sizeof_fmt(num, suffix='B'): | |||
num /= 1024.0 | |||
return fmt % (num, 'Y', suffix) | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference is to remove the pyfiglet
font and just report the pricing in the default font.
pandas_gbq/gbq.py
Outdated
@@ -421,6 +439,17 @@ def sizeof_fmt(num, suffix='B'): | |||
num /= 1024.0 | |||
return fmt % (num, 'Y', suffix) | |||
|
|||
def query_price_for(self, bytes_num): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a unit test for query_price_for
so that our code coverage doesn't decrease?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
8ee1efd
to
fd5e0e6
Compare
now it's clean and easy! please revie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tworec !
My only suggestion would be to include '$'
before the price and also include the currency but I'm ok with the current version.
@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