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.
| # I don't think the result of this code ever gets used, nothing looking for "ABORT_FORCE" | ||
| # unpacks from a tuple. This can probably be deleted. |
There was a problem hiding this comment.
It's possible that that was added after I started this work. The blame says it's a similar age. I've removed the comments and adjusted the queue additions, we're not using the ("Result", "Message") format anymore.
|
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.
| from collections import OrderedDict | ||
|
|
||
|
|
||
| T = TypeVar("T") |
There was a problem hiding this comment.
There was a problem hiding this comment.
Switched to the alternate method of defining generics. Looks cleaner too IMO.
There was a problem hiding this comment.
Except now it looks like I will need to bump the version of black to get it through
There was a problem hiding this comment.
And now that I've done that and with tox doing its thing (which I didn't run locally before pushing because dumb that's why) the CI run is failing because of other things that the newer version of black wants to introduce. This will be a later problem now.
There was a problem hiding this comment.
Ok, so this rang a bell from when I was doing the ftdi driver update that is being put into a separate repo under PyFixate (coming soonTM). @clint-lawrence and I spoke about either getting rid of the tox black environment or running pre-commit in tox as well.
I think after speaking to @daniel-montanari about this also, I'm leaning towards just removing the black section from tox. This means that we won't have bulk formatting changes coming in on an ostensibly unrelated PR. If any unchanged files are touched in the future, then these will be caught up with the newer black enforcements.
There was a problem hiding this comment.
Bumped black and reran #261
I found this pretty easy to merge into my vmux branch. The only conflicts I had were the config files I had touched and my code that had not been run through the new version of black.
If this ends up being a pain to merge for you then I'm happy to abandon that PR. Really it just means a few extra whitespace diffs when someone touches a file that hasn't been formatted in a while.
|
@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
| from collections import OrderedDict | ||
|
|
||
|
|
||
| T = TypeVar("T") |
There was a problem hiding this comment.
Bumped black and reran #261
I found this pretty easy to merge into my vmux branch. The only conflicts I had were the config files I had touched and my code that had not been run through the new version of black.
If this ends up being a pain to merge for you then I'm happy to abandon that PR. Really it just means a few extra whitespace diffs when someone touches a file that hasn't been formatted in a while.
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.