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

Added percentage next to failure count in output #30

Merged
merged 7 commits into from
Nov 10, 2016
Merged

Added percentage next to failure count in output #30

merged 7 commits into from
Nov 10, 2016

Conversation

sgpeter1
Copy link
Contributor

@sgpeter1 sgpeter1 commented Nov 8, 2016

Simple changes to add failure percentage inline with failure count. Feedback, editing, and mentoring always welcome. :)

Fixes #29 Percentage for failure counts.

@sgpeter1 sgpeter1 closed this Nov 8, 2016
@sgpeter1 sgpeter1 reopened this Nov 8, 2016
Copy link
Owner

@di di left a comment

Choose a reason for hiding this comment

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

Looks good but I think there's a small error in the percentage calculation. I'd recommend running the examples in https://github.com/di/vladiate/blob/master/vladiate/examples/vladfile.py to see if the percentages make sense.

@@ -85,6 +86,7 @@ def validate(self):
return False

for line, row in enumerate(reader):
self.line_count = line
Copy link
Owner

Choose a reason for hiding this comment

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

This will set self.line_count multiple times until the loop has finished, and since enumerate produces a zero-indexed list, this causes an off-by-one error where self.line_count is one less than the actual total number of lines.

Instead, you might want to do self.line_count += 1.

Corrected line counting logic to accurately reflect the number of lines actually parsed.
@sgpeter1
Copy link
Contributor Author

Thanks for the feedback and help!

validator.__class__.__name__, validator.fail_count,
field_name))
validator.fail_count/self.line_count, field_name))
Copy link
Owner

Choose a reason for hiding this comment

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

Another issue here is that in Python 2, dividing two integers with / will produce another integer, so this will always be either 1 or 0. For example:

$ python
Python 2.7.12 (default, Oct 25 2016, 12:32:25)
[GCC 4.2.1 Compatible Apple LLVM 7.3.0 (clang-703.0.31)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> 1/4
0

In Python 3, this has been changed, so at the top of this file you can add the following and we'll have floats instead:

>>> from __future__ import division
>>> 1/4
0.25

Apparently it really matters to have the __future__ imports at the top.
@di
Copy link
Owner

di commented Nov 10, 2016

Thanks @sterlingpetersen! And congratulations on your first PR! 🎉

@di di merged commit 847f53e into di:master Nov 10, 2016
@sgpeter1
Copy link
Contributor Author

Thanks for your patient mentoring @di!

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.

2 participants