Revamp UI interactions#214
Conversation
jcollins1983
left a comment
jcollins1983
left a comment
There was a problem hiding this comment.
I started answering comments in VS Code and it decided that I was doing a review... anyhow... here we are.
I think I've addressed all the things you raised. I've left the conversations as unresolved, as I like to leave that to the reviewer to determine. I also added a file that I used for manual testing of how the UI is displayed.
|
This is ready to be reviewed again. |
daniel-montanari
left a comment
daniel-montanari
left a comment
There was a problem hiding this comment.
Old tests still pass.
New tests seem ok.
A few comments to talk over but nothing majorly wrong.
|
@daniel-montanari I think I've addressed all of your comments. There were some outdated comments that I marked as resolved, but there are still a few comments where I responded and in general, I prefer the reviewer to mark as resolved if they're happy with the response - so I've left them for you to mark as resolved if you're happy. |
daniel-montanari
left a comment
daniel-montanari
left a comment
There was a problem hiding this comment.
Check CI, minor comments otherwise.
Will need to pull latest changes into this to fix failing tests.
What failing tests? And what specifically in the CI? Vague comments don't help in the resolution of these things... |
|
@daniel-montanari I've addressed the conflict, tests are passing. There are a whole bunch of unresolved comments in outdated sections that I would like you to either provide further comment on or resolve if you're happy. With regard to the formatting. It is currently reliant upon pre-commit and the person doing any work having the pre-commit hooks installed. I'm open to including pre-commit in the actions to use the same configuration that each person should be using when making changes in a branch. The reason I left it out is that when we need to bump versions of black etc. to accommodate for new features required for the code we're modifying we don't end up with the entire codebase being updated "just because", I would prefer to defer this to when/if this code needs to be modified in the future, but if there's enough desire from others to continue to format the entire codebase each time the formatting tools are updated then I'll acquiesce. |
I should've been more specific. |
daniel-montanari
left a comment
daniel-montanari
left a comment
There was a problem hiding this comment.
Will approve pending your decision of what to do with the formatting.
Can either have a go merging my changes in or abandon and go with these changes.
#214 (comment)
#261
3 participants
This PR addresses Issue #177 and Issue #213.
I've done a similar thing to the switching module and put the new _ui.py module at the fixate level and have only exposed the public functions.
To avoid having 2 different sets of functions in
cmd_line.pyandui_gui_qt.pyI moved the (duplicated) logic out of the UI files into ui.py and kept the original behaviour, tests were updated to deal with this change.Also added the ability to have colour on
user_info_importantthe line of!defaults to red, which will hopefully be a bit more eye catching.Have tested each of the functions manually. But should try on a real script or 2 before this gets merged if it does.