Skip to content
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

Fix typings for addition in eth_sum #5243

Merged
merged 6 commits into from
Oct 2, 2019
Merged

Fix typings for addition in eth_sum #5243

merged 6 commits into from
Oct 2, 2019

Conversation

edsonayllon
Copy link
Contributor

@edsonayllon edsonayllon commented Sep 24, 2019

Description

This provides a fix, converting tips to type float before adding to eth_sum.

eth_sum did not include tips received by default. When tips were added, they were kept in a type incompatible for addition with a non-empty eth_sum. No error was thrown when a profile only had tips or only had completed bounties, but threw an error for profiles with both tips and full bounty rewards. This provides a fix to that error.

Refers/Fixes

#5109

#5191

Testing

Most testing was done with print() in the eth_sum function in models.py by loading a page calling eth_sum while docker was in the foreground. Types were logged with print(). eth_sum was filled with a float variable to test for addition compatibility.

@edsonayllon
Copy link
Contributor Author

@thelostone-mc This PR provides a fix to the previous PR. However, this issue doesn't seem to be a problem with the new profiles released. I added the PR regardless in case eth_sum is used in the future.

@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #5243 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5243      +/-   ##
==========================================
- Coverage   29.89%   29.89%   -0.01%     
==========================================
  Files         230      230              
  Lines       18997    18999       +2     
  Branches     2709     2711       +2     
==========================================
  Hits         5680     5680              
- Misses      13080    13082       +2     
  Partials      237      237
Impacted Files Coverage Δ
app/dashboard/models.py 50% <0%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a0352...11a5069. Read the comment docs.

@danlipert
Copy link
Contributor

@edsonayllon why not use the list comprehension as before? Seems like the right way to go, just some issues with the typing

@edsonayllon
Copy link
Contributor Author

@danlipert Both do the same thing. However, having it all on one line resulted in syntax errors when messing with multiple parenthesis, which made debugging a bit frustrating. So for legibility, I expanded the logic.

@edsonayllon
Copy link
Contributor Author

@danlipert I returned it to a list comprehension, and reduced the if statement.

@thelostone-mc thelostone-mc merged commit 3dcb05e into gitcoinco:master Oct 2, 2019
@octavioamu
Copy link
Contributor

Again failing on production 😞

@edsonayllon
Copy link
Contributor Author

edsonayllon commented Oct 3, 2019

@octavioamu

Same error? That would mean bounties[n].get_value_in_eth is type decimal.Decimal, and self.tip[n].value_in_eth is of type float already.

eth_sum = eth_sum + sum([ float(amount.value_in_eth) for amount in self.tips ])

Would need to be changed to:

eth_sum = eth_sum + sum([ Decimal(amount.value_in_eth) for amount in self.tips ])

Or

eth_sum += sum([ Decimal(amount.value_in_eth) for amount in self.tips ])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants