-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactoring #9
Refactoring #9
Conversation
I think some explanations can be useful:
|
@WnP you're killing it! I'll check this out, and will hopefully have it merged by EOD. Thanks! |
@lukasschwab @WnP This looks really good, now its starting to look like some clean python code. The reason for that ugly try, except stuff was to just band aid the problem temporarily. But a PR like this was something I had in mind in the near future for this project, good job @WnP! Looks great. Also if you are interested @WnP, have a look at a discussion we were having a few days ago. And chime in if you'd like. Since this is taken care of, next I want to look into this problem and where |
@WnP I encountered an error dropping back from the question focus view into the list view. The problem arises because the list view is generated starting at the selected question upon return––e.g., if I'm looking at the list view for questions 1-5, select 4, and then enter There isn't appropriate handling for cases where the script attempts to print more questions than exist in the list. This is not an issue, I believe, that existed before these changes. Here's the CLI interaction, with some abridgments marked by
|
@lukasschwab this - aaed221 - fix the bug on listing, nevertheless it no longer list questions 4 to 9 after a selecting question 4, now it returns questions 1 to 5 I think it's possible to return 4 to 9, but it required some more change which I guess are out of this PR I've fixed another bug on @schwartzmx I'm going to read that thread and give you some feedback, thanks ;-) |
Did some testing, this looks great! To avoid this kind of confusion in the future, it would be great to separate refactoring from functionality changes––makes testing easier, as functionality changes in refactoring would normally raise a red flag. I'm going to move forward with a merge and a new release. |
Refactoring + PEP8 compliance
yes, I agree that normally I should made another PR for this functionality thanks for merging |
@lukasschwab the gif demo in maybe we should show a message that tell the user it must read the usage using |
@lukasschwab you could find an up to date gif here if it fits your needs |
@WnP you're solving problems as fast as I can add them to my to-do list! Adding to README.md |
Sorry ^^ Thx |
I've made a lot of change in the source code to make it more pythonic (pep8 included), readable and maintenable
If you have some question let me know