Re: Google finance support for googlecl (issue2412041)
thmi...@google.com
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py
File src/googlecl/finance/service.py (right):
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py#newcode95
src/googlecl/finance/service.py:95: return pfl
On 2010/10/10 17:30:49, Ed Bartosh wrote:
> On 2010/10/09 21:17:03, thmiller wrote:
> > Did you look at using googlecl.base.BaseCL.GetEntries() to do this?
If you
> only
> > need exactly one portfolio, use GetSingleEntry().
> I doubt it will be much better. Looks like I will end up
reimplementing
> Portfolio query.
You should be able to use the PortfolioQuery, just call ToUri() on it
before it gets sent to GetEntries. (see docs/service.py)
GetEntries will do that title matching for you, support regular
expression matches on the title, convert non-latin characters safely to
the URL, etc.
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py#newcode162
src/googlecl/finance/service.py:162: print fmt % ('Id', 'Title', 'Curr',
'Gain', 'Gain %', 'C.basis',
On 2010/10/10 17:30:49, Ed Bartosh wrote:
> On 2010/10/09 21:17:03, thmiller wrote:
> > You should let the user decide what fields to list with the --fields
option.
> > picasa/service.py and contacts/base.py contain pretty extensive
wrapper
> classes
> > to base yours off of, or you can choose another way to do it.
> Done.
OK, the formatter looks pretty good. I may merge the two methods in the
future.
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py#newcode232
src/googlecl/finance/service.py:232:
client.DeleteTransaction(transaction)
On 2010/10/10 17:30:49, Ed Bartosh wrote:
> On 2010/10/09 21:17:03, thmiller wrote:
> > Take a look at DeleteTransaction. I bet we can use DeleteEntry
instead, which
> is
> > sometimes helpful.
> I'm not sure I understand you here. Why using specific API should be
avoided in
> favor of more low-level one? I think it's more readable and
informative to use
> DeleteTransaction here. Moreover, if they change internals of
DeleteTransaction
> we shouldn't do anything, but if we use DeleteEntry something might
break in
> such a case.
All of the DeleteSpecificEntry functions in gdata are just wrappers to
Delete. Actually, in Finance's case, poorly defined wrappers -- you can
easily blunder into an AttributeError with DeletePortfolio. You're right
that it's bad practice to make the assumption, but it's been that way
since 1.2.4, and I don't expect that style to change any time soon.
delete_entry_list will also prompt the user before deleting something,
and allow a list of things to delete. As it's written now, the user has
to call _run_delete_transaction for every transaction deleted.
http://codereview.appspot.com/2412041/diff/8001/src/googlecl/finance/service.py#newcode342
src/googlecl/finance/service.py:342:
This is an instance where it pays to use DeletePosition over googlecl's
delete_entry_list, though that means that a "are you SURE?" prompt needs
to be re-written for this case. Or delete_entry_list gets re-written
with a callback, instead of using Delete() every time.
thmi...@google.com
The comment on base.py still stands, but that's separate from adding
finance, so I'll make separate commits with changes to base.py that
support deletion callbacks and no max-results.
http://codereview.appspot.com/2412041/diff/13001/src/googlecl/base.py
File src/googlecl/base.py (right):
http://codereview.appspot.com/2412041/diff/13001/src/googlecl/base.py#newcode197
src/googlecl/base.py:197: if set_maxresults:
If you really don't want to include a max-results parameter for Finance,
could you set self.max_results = None in the constructor instead of
adding a parameter to get_entries?
bar...@gmail.com
Thank you for your suggestions.
I reimplemented getting portolio using GetEntries and Getsingleentry and
all delete* methods using DeleteEntryList as you suggested. Please
review.
Regards,
Ed
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py
File src/googlecl/finance/service.py (right):
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py#newcode95
src/googlecl/finance/service.py:95: return pfl
On 2010/10/09 21:17:03, thmiller wrote:
> Did you look at using googlecl.base.BaseCL.GetEntries() to do this? If
you only
> need exactly one portfolio, use GetSingleEntry().
Done.
http://codereview.appspot.com/2412041/diff/1/src/googlecl/finance/service.py#newcode232
src/googlecl/finance/service.py:232:
client.DeleteTransaction(transaction)
On 2010/10/09 21:17:03, thmiller wrote:
> Take a look at DeleteTransaction. I bet we can use DeleteEntry
instead, which is
> sometimes helpful.
Done.
http://codereview.appspot.com/2412041/diff/8001/src/googlecl/finance/service.py#newcode342
src/googlecl/finance/service.py:342:
On 2010/10/13 21:59:44, thmiller wrote:
> This is an instance where it pays to use DeletePosition over
googlecl's
> delete_entry_list, though that means that a "are you SURE?" prompt
needs to be
> re-written for this case. Or delete_entry_list gets re-written with a
callback,
> instead of using Delete() every time.
Done.
bar...@gmail.com
http://codereview.appspot.com/2412041/diff/13001/src/googlecl/base.py
File src/googlecl/base.py (right):
http://codereview.appspot.com/2412041/diff/13001/src/googlecl/base.py#newcode197
src/googlecl/base.py:197: if set_maxresults:
On 2010/10/14 18:14:44, thmiller wrote:
> If you really don't want to include a max-results parameter for
Finance, could
> you set self.max_results = None in the constructor instead of adding a
parameter
> to get_entries?
I'd love to leave it like it was, but finance service doesn't support
this parameter. This is what I got when trying to use GetEntries
whithout modifications: "Failed to get entries: {'status': 403, 'body':
'This service does not support the 'max-results' parameter.',
'reason': 'Forbidden'}". I'm not sure that having attribute is better
than having explicit parameter, but if you insist I'll do that.
bar...@gmail.com
http://codereview.appspot.com/2412041/diff/13001/src/googlecl/base.py
File src/googlecl/base.py (right):
http://codereview.appspot.com/2412041/diff/13001/src/googlecl/base.py#newcode197
src/googlecl/base.py:197: if set_maxresults:
On 2010/10/14 18:14:44, thmiller wrote:
> If you really don't want to include a max-results parameter for
Finance, could
> you set self.max_results = None in the constructor instead of adding a
parameter
> to get_entries?
Done.