Skip to content

Conversation

@FinMacDov
Copy link
Contributor

Thank you for your contribution. Please provide the details requested below.

ISSUE NUMBER

#106.

SHORT DESCRIPTION

Added import random and added function for placing (this was missing and preventing code to run).

TESTING

The code now outputs the histograms. Output needs double check its correct.

Added import random and added function for placing.
@pep8speaks
Copy link

pep8speaks commented May 10, 2018

Hello @FinMacDov! Thanks for updating the PR.

Line 11:1: E305 expected 2 blank lines after class or function definition, found 1
Line 38:1: E305 expected 2 blank lines after class or function definition, found 1
Line 50:1: W293 blank line contains whitespace
Line 51:1: E305 expected 2 blank lines after class or function definition, found 1
Line 63:1: W293 blank line contains whitespace
Line 64:1: E305 expected 2 blank lines after class or function definition, found 1
Line 76:1: W293 blank line contains whitespace
Line 77:1: E305 expected 2 blank lines after class or function definition, found 1
Line 94:1: E305 expected 2 blank lines after class or function definition, found 1
Line 99:1: W293 blank line contains whitespace
Line 127:1: E302 expected 2 blank lines, found 1
Line 140:1: E305 expected 2 blank lines after class or function definition, found 1

Comment last updated on May 11, 2018 at 02:17 Hours UTC

@FinMacDov
Copy link
Contributor Author

Further improvements would be to add labels to the histograms. Also for the vertical axis would be more readable if it was in terms of percentage.

@prateekiiest
Copy link
Owner

could you show me the output here pls, by running this on your local machine.
Kudos again for opening this PR :)

@FinMacDov
Copy link
Contributor Author

FinMacDov commented May 10, 2018

Not that I was aware of. I am fairly new to contributing to other people projects. What am I better to do in future for making the pull requests? Here is the output
hist_1
hist_2

@prateekiiest
Copy link
Owner

you are doing just great @FinMacDov . Thanks for the effort.
Before I merge in it would be great if you can fix those pep8 issues.

@prateekiiest prateekiiest mentioned this pull request May 10, 2018
@FinMacDov
Copy link
Contributor Author

Working on the pep8 issues.

@prateekiiest
Copy link
Owner

lemme know if you are facing any issues with pep8 fix

@FinMacDov
Copy link
Contributor Author

That's the pep8 issues resolved. Let me know if there anything else you want to change with the code.

@prateekiiest
Copy link
Owner

Some more pep8 issues got involved

@prateekiiest
Copy link
Owner

I am merging this as of now

@prateekiiest prateekiiest merged commit e43fd47 into prateekiiest:master May 11, 2018
@FinMacDov FinMacDov deleted the testing branch May 11, 2018 08:19
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.

3 participants