Fix RatesQuery#rates method to respect relevant_date argument #2399
Conversation
erkde
force-pushed
the
fix_rates_query_rates
branch
from
d026c95 to
bc716c3
Compare
|
Was commit ed1e6fe (titled "Rename rates local shadowing enclosing method name") necessary? Or was it more of a good practice change? Aside from my question, this all looks good to me. |
|
@chrisroos - I don't think it is necessary, just my preference given the enclosing method signature is the same as the local variable, when called without an argument (using the default value). I just checked the shadowing lint in Rubocop and it seems like it doesn't apply, so can probably be safely removed from the PR. |
|
Btw, I'm can no longer merge my PR's, not even sure about pushing a rebase of my branches to |
|
Thanks for clarifying, @erikse. I'm happy for the first commit to remain, I was simply interested in the motivation. I'll get this rebased and merged shortly. |
Caching @rates with the first calculated result doesn’t allow for the #rates method to return correct values for alternate dates. As the #data method has its own memoization of the YAML file content after being invoked the first time, searching through the contents on subsequent invocations of #rates hopefully isn’t too costly.
This class is used by several smart answers, so I’m not sure why its file checksum is tracked for this smart answer particularly.
chrisroos
force-pushed
the
fix_rates_query_rates
branch
from
bc716c3 to
1ba86a1
Compare
|
I've rebased this on master and force pushed in preparation for merging. I was happy to force push as there were no comments on individual commits that would be lost. |
|
Cool, thanks for the help merging On Wednesday, 30 March 2016, Chris Roos notifications@github.com wrote:
Sent from Gmail Mobile |
There seems to be a subtle bug in this code, in that the query class saves the results of invoking #rates the first time in a @rates instance variable.
But on subsequent invocations, it returns the previously calculated value, regardless if it is invoked with a different
relevant_dateargument.