MasterMedo / typetest

:chart_with_upwards_trend: Improve your typing speed without leaving the terminal.
MIT License
24 stars 8 forks source link

Prints a message when there's not enough data to analyse #32

Closed yagueto closed 3 years ago

yagueto commented 3 years ago

Right now, the program crashed when there was not enough data/the files were not found. This solves it by checking the file existence and takes into account what graph was requested, so that it only asks for the absolutely needed files.

Please complete these tasks before opening your PR:

Fixes #25

MasterMedo commented 3 years ago

Thanks for contributing! 🎉

I skimmed a bit through the approach you took. I don't know if that's better than what I had in mind. I thought of keeping the defaults in command line arguments because they are explicit in typetest --help. The idea was to make a decorator for every plot function, take the string filename argument from it, and convert it to a file if it exists. I'll think about it for a day, and get back to you. 😁

yagueto commented 3 years ago

Hey! This approach still uses the default values if no other values have been provided:

https://github.com/yagueto/typetest/blob/58183d2057172f1b671577f4823ecf50651f746a/typetest/utils.py#L60

While your approach seems good, the issue is that if you don't provide an argument and the files do not exist, then argparse will exit the program when parsing the arguments (no exception, just a bare sys.exit), making it difficult to use a try block.

MasterMedo commented 3 years ago

You're right 😄, with the current implementation argparse throws an error when trying to open a file if the file doesn't exist. I hinted at changing the type of the command line argument; from FileType("r") to str. So that instead of passing a file to the plot functions, we would pass a

string filename argument

(I'm quoting myself) 😅

I think the wrapper should have absolutely no knowledge where the test results are stored. It's a helper function. It checks if the arguments are good, if not, the program exits with a friendly message.

Btw, don't take this as an ultimatum, I still haven't taken the time to think about the consequences of each approach. ^^

Please continue the discussion if you think the approach you took is still superior to the one I had in mind.

yagueto commented 3 years ago

Ahh okay, hadn't quite got it 😅.

It seems like a good idea to me, I'll try to work on it

MasterMedo commented 3 years ago

Now this looks very promising! I'll test it now and report back :3

MasterMedo commented 3 years ago

Thanks for contributing, once more! 🎉

It was nice working with you. :3 I'll merge this now. Cheers!

yagueto commented 3 years ago

I took the liberty to fix the bugs as I encountered them

Woops, sorry, should have checked it in more depth 😅

Thank you too for the review, it was great to compare different points of view!