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 bug on sourced percentage, add test #78

Merged
merged 3 commits into from
Dec 5, 2014

Conversation

lepistone
Copy link
Member

Because of a integer division error, with one sourced line out of two,
the sourcing percentage was zero instead of 0.5. Added a test to check
this case.

@jgrandguillaume
Copy link
Member

👍 when travis green ! Thanks

@lepistone
Copy link
Member Author

Red because of lint. I'm fixing it.

@lepistone
Copy link
Member Author

lint fixed

if req.state != 'cancel')
sourced_len = sum(1 for req in requisition.line_ids
sourced_len = sum(1.0 for req in requisition.line_ids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the record, adding from __future__ import division at the beginning of the file is a nicer way of fixing this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't knew that one... thanks !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and you can use the // operator if you still want integer division.

@lepistone
Copy link
Member Author

@gurneyalex welcome to the future.

Because of a integer division error, with one sourced line out of two,
the sourcing percentage was zero instead of 0.5. Added a test to check
this case.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling 2c855be on lepistone:fix-float-source into 14c3c5c on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling a6a83e6 on lepistone:fix-float-source into 721b681 on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling a6a83e6 on lepistone:fix-float-source into 721b681 on OCA:8.0.

@foutoucour
Copy link

👍

@bwrsandman
Copy link

👍 Very nice: one line change and he still adds tests 👏

@guewen
Copy link
Member

guewen commented Dec 5, 2014

👍

guewen added a commit that referenced this pull request Dec 5, 2014
fix bug on sourced percentage, add test
@guewen guewen merged commit 35cc645 into OCA:8.0 Dec 5, 2014
@guewen
Copy link
Member

guewen commented Dec 5, 2014

Hmm. It got red once merged 😠

@lepistone
Copy link
Member Author

We're having some flickering tests runs here and there, and I haven't investigated the reason. If someone finds out what's going on please make some noise!

@lepistone
Copy link
Member Author

I found out that this is not flickering at all, instead there is a genuine mistake of mine in this PR that we all didn't see because of the false negatives we had 2 weeks ago with MQT. These problems are fixed now, so we made the whole vertical-ngo/8.0 branch red.

I'm preparing a PR to sort that out and I'll ask reviewers to fast-track it.

@lepistone lepistone deleted the fix-float-source branch December 8, 2014 13:24
@lepistone
Copy link
Member Author

Fixed in #93

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

Successfully merging this pull request may close these issues.

8 participants