-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add typing checks to ui #45
Conversation
goanpeca
commented
Nov 29, 2022
•
edited
Loading
edited
- Add a bit of typing
- Add typing checks
- Fix resource loading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @goanpeca , left some comments/info on the typing check workflow and the images.py
file and its usage. Other than that this LGTM 👍
Thanks for the comments @dalthviz, will fix them! |
@@ -4,6 +4,8 @@ on: | |||
pull_request: | |||
branches: | |||
- main | |||
paths: | |||
- 'build_installers.py' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about the rationale behind constraining the runs to this file only. otherwise PR lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @potating-potato, so we do not want to trigger a bundle build unless something changed in the file in charge of creating the bundles. I added this on this PR to make the other PRs simpler.
constructor-manager-ui/src/constructor_manager_ui/style/utils.py
Outdated
Show resolved
Hide resolved
# Generate resources file from QRC file | ||
print("Generating resources file from QRC file...") | ||
check_call(["pyrcc5", "images.qrc", "-o", "images.py"], cwd=CWD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is the assumption that it will be built using PyQt5? What with PyQt6? PySide[2|6]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, but will update in a separate PR to use the correct binding found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a TODO or FIXME here in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created this issue #47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a todo also ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @goanpeca !