Skip to content
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

Merged
merged 15 commits into from
Nov 29, 2022
Merged

Add typing checks to ui #45

merged 15 commits into from
Nov 29, 2022

Conversation

goanpeca
Copy link
Collaborator

@goanpeca goanpeca commented Nov 29, 2022

  • Add a bit of typing
  • Add typing checks
  • Fix resource loading

@goanpeca goanpeca marked this pull request as ready for review November 29, 2022 04:33
@goanpeca goanpeca self-assigned this Nov 29, 2022
Copy link
Member

@dalthviz dalthviz left a 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 👍

.github/workflows/typing.yml Show resolved Hide resolved
.github/workflows/typing.yml Outdated Show resolved Hide resolved
.github/workflows/typing.yml Outdated Show resolved Hide resolved
@goanpeca
Copy link
Collaborator Author

Thanks for the comments @dalthviz, will fix them!

@@ -4,6 +4,8 @@ on:
pull_request:
branches:
- main
paths:
- 'build_installers.py'
Copy link
Contributor

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

Copy link
Collaborator Author

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/data.py Outdated Show resolved Hide resolved
Comment on lines 52 to 54
# Generate resources file from QRC file
print("Generating resources file from QRC file...")
check_call(["pyrcc5", "images.qrc", "-o", "images.py"], cwd=CWD)
Copy link

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]?

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created this issue #47

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a todo also ;)

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @goanpeca !

@goanpeca goanpeca merged commit 75f01b1 into napari:main Nov 29, 2022
@goanpeca goanpeca deleted the typing branch November 29, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants