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

Initial napari specific classes for package installer related logic #121

Merged
merged 6 commits into from
Dec 30, 2024

Conversation

dalthviz
Copy link
Member

Part of #59

@dalthviz dalthviz self-assigned this Dec 13, 2024
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 94.03409% with 21 lines in your changes missing coverage. Please review.

Project coverage is 94.07%. Comparing base (84f79a0) to head (b7faa65).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
napari_plugin_manager/base_qt_package_installer.py 92.95% 20 Missing ⚠️
napari_plugin_manager/qt_package_installer.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   93.53%   94.07%   +0.53%     
==========================================
  Files          11       13       +2     
  Lines        1981     2026      +45     
==========================================
+ Hits         1853     1906      +53     
+ Misses        128      120       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dalthviz dalthviz marked this pull request as ready for review December 17, 2024 18:18
@dalthviz dalthviz requested a review from jaimergp December 17, 2024 18:19
@jaimergp jaimergp requested a review from goanpeca December 20, 2024 15:39
@jaimergp
Copy link
Contributor

LGTM but let's have @goanpeca take a look too before the merge.

Copy link
Contributor

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

Hi @dalthviz just went through the PR. Looks really good :)

Thanks for working on this.

Is there anything that can be tested from the base implementations as to have separate test files also?

test_base_qt_installer...
test_qt_installer...

?

@dalthviz
Copy link
Member Author

Is there anything that can be tested from the base implementations as to have separate test files also?

test_base_qt_installer...
test_qt_installer...

?

Thinking about that maybe we could move

def test_not_implemented_methods():
tool = AbstractInstallerTool('install', ['requests'])
with pytest.raises(NotImplementedError):
tool.executable()
with pytest.raises(NotImplementedError):
tool.arguments()
with pytest.raises(NotImplementedError):
tool.environment()
with pytest.raises(NotImplementedError):
tool.available()

to a test_base_qt_installer. Will do that change 👍

@jaimergp
Copy link
Contributor

Looks like we have a reproducible error in the Windows CI 🤔

@dalthviz
Copy link
Member Author

I think I was able to fix it by adding a qtbot.wait but let me know if something else is needed here!

@dalthviz
Copy link
Member Author

Should we merge this now and proceed with releasing v0.1.4 @jaimergp @goanpeca ? Or something else is missing?

@goanpeca
Copy link
Contributor

I have a couple of PRs that I will merge afterwards, but I can also make a new minor release anyway :)

So either way works for me

@dalthviz
Copy link
Member Author

dalthviz commented Dec 30, 2024

Oh I see, I think I will merge then this in an hour or so (if no other comment is added). Also, maybe we should list/mention those other pending PRs over #122 ? Makes sense to me to merge any pending work and just do one new release 🤔

@dalthviz dalthviz merged commit 05728e0 into napari:main Dec 30, 2024
20 checks passed
@goanpeca
Copy link
Contributor

Will work on those PRs today and will ping you back :)

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.

3 participants