aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
7 stars 17 forks source link

fix bug report to use the new return type of find_installed_packages #446

Closed unkcpz closed 1 year ago

unkcpz commented 1 year ago

fixes #445

Note: after this PR is merged, it needs to backport to 1.4.x as well.

danielhollas commented 1 year ago

@unkcpz just a quick question, is your fix compatible with both the old and new aiidalab package versions? I think it needs to be, otherwise it's a mess for the users.

unkcpz commented 1 year ago

@unkcpz just a quick question, is your fix compatible with both the old and new aiidalab package versions?

Nope, but I see your point. I'll make a change. Thanks for the head up.

unkcpz commented 1 year ago

It support the old version of aiidalab. But the test_fingerprint_parser pass without hitting the correct code path. In the test, it still using the old aiidalab since we didn't release a new version yet.

This is a issue with unit test because of the load_profile() we used in __init__.py, I don't have very clear clue why that happened but that need the fix from aiida-core I assume. The workaround is moving the import clause into the test functions. @yakutovicha encounter the same issue in his PR, we need investigate more and then open the issue in aiida-core.

For this PR, I think it is ready to review.

unkcpz commented 1 year ago

I also move the fixtures in this PR with are duplicate as @yakutovicha did in https://github.com/aiidalab/aiidalab-widgets-base/pull/441/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128 Let's discuss how we manage these on Thursday.

yakutovicha commented 1 year ago

I also move the fixtures in this PR with are duplicate as @yakutovicha did in https://github.com/aiidalab/aiidalab-widgets-base/pull/441/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128 Let's discuss how we manage these on Thursday.

I wouldn't worry too much about this. If your PR is merged first, I will adapt mine. If mine - then the other way around. It is really about moving some chunk of code, nothing more than this.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.47 :tada:

Comparison is base (76e31cb) 39.21% compared to head (9fa6dff) 39.69%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #446 +/- ## ========================================== + Coverage 39.21% 39.69% +0.47% ========================================== Files 18 20 +2 Lines 3091 3162 +71 ========================================== + Hits 1212 1255 +43 - Misses 1879 1907 +28 ``` | Flag | Coverage Δ | | |---|---|---| | python-3.10 | `39.69% <100.00%> (+0.47%)` | :arrow_up: | | python-3.8 | `39.70% <100.00%> (+0.47%)` | :arrow_up: | | python-3.9 | `39.70% <100.00%> (+0.47%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [aiidalab\_widgets\_base/bug\_report.py](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-YWlpZGFsYWJfd2lkZ2V0c19iYXNlL2J1Z19yZXBvcnQucHk=) | `53.33% <100.00%> (ø)` | | | [tests/test\_bug\_report.py](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9idWdfcmVwb3J0LnB5) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

unkcpz commented 1 year ago

I am aware that this might not by DRY or whatever, but let's be pragmatic here and define our own simplified local find_installed_packages (without using the Package class). WDYT?

I was worried about it not DRY, but after trying it is very clean. After all, we just need a very simple help function.

unkcpz commented 1 year ago

Will you take care of cherry-picking this on the 1.x branch and releasing a new version?

Sure I'll do that now.