Skip to content

Fix typing checks in PRs#6230

Merged
Carreau merged 1 commit intonapari:mainfrom
andy-sweet:fix-typing
Sep 19, 2023
Merged

Fix typing checks in PRs#6230
Carreau merged 1 commit intonapari:mainfrom
andy-sweet:fix-typing

Conversation

@andy-sweet
Copy link
Copy Markdown
Member

@andy-sweet andy-sweet commented Sep 18, 2023

#6203 was merged without updating the latest changes from main and now the typing checks in PRs fails due to updates in napari.components. We don't run those changes on main, though maybe we should given that we don't requires PR branches to be up-to-date before merging (only that git reports no conflicts).

This PR fixes the typing failure by casting the type away from the possible value of None, which is a side-effect of calling _open_preferences_dialog. An alternative fix is to have _open_preferences_dialog to return PreferencesDialog, which I like but felt like a bigger change than needed. Of course, we can also use # type: ignore, but I don't there's a need here.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 18, 2023

Codecov Report

Merging #6230 (629ad11) into main (7099b18) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #6230   +/-   ##
=======================================
  Coverage   91.60%   91.60%           
=======================================
  Files         583      583           
  Lines       51363    51364    +1     
=======================================
+ Hits        47050    47054    +4     
+ Misses       4313     4310    -3     
Files Changed Coverage Δ
napari/components/_viewer_key_bindings.py 96.51% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Copy Markdown
Contributor

@DragaDoncila DragaDoncila left a comment

Choose a reason for hiding this comment

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

@andy-sweet agree returning the dialog would be nice. Definitely approving for now though so we can get PRs green again

@DragaDoncila DragaDoncila added the ready to merge Last chance for comments! Will be merged in ~24h label Sep 19, 2023
@Carreau Carreau added bug Something isn't working as expected bugfix PR with bugfix and removed bug Something isn't working as expected labels Sep 19, 2023
@Carreau Carreau modified the milestone: 0.5.0 Sep 19, 2023
@Carreau
Copy link
Copy Markdown
Contributor

Carreau commented Sep 19, 2023

There seem to be a GitHub bug that

  1. don't retrigger action on label/milestone change.
  2. don't see the milestone when re-triggered manually.

I'm leaning to merge even if "Label and milestone checker" is read to fix other PRs.

@lucyleeow
Copy link
Copy Markdown
Contributor

agree returning the dialog would be nice.

Made an issue #6235 so we can keep track

@Czaki Czaki added ready to merge Last chance for comments! Will be merged in ~24h and removed ready to merge Last chance for comments! Will be merged in ~24h labels Sep 19, 2023
@Czaki
Copy link
Copy Markdown
Collaborator

Czaki commented Sep 19, 2023

There seem to be a GitHub bug that

1. don't retrigger action on label/milestone change.

Now it works. maybe some problem with their internal Api at this time

2. don't see the milestone when re-triggered manually.

We check milestones based on event payload, not rest API requests. It restarted with an identical event payload. So restart will not help in such case.

I'm leaning to merge even if "Label and milestone checker" is read to fix other PRs.

All green. I will merge in few hours.

@Carreau
Copy link
Copy Markdown
Contributor

Carreau commented Sep 19, 2023

All green. I will merge in few hours.

Exceptionally as this is a hotfix, I'm going to merge to unblock/rebase other failing PRs.

@Carreau Carreau merged commit eaff277 into napari:main Sep 19, 2023
@andy-sweet andy-sweet deleted the fix-typing branch September 19, 2023 14:09
kne42 added a commit to kne42/napari that referenced this pull request Sep 19, 2023
* main:
  Fix typing checks in PRs (napari#6230)
@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PR with bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants