search

aiidalab / aiidalab-widgets-base

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

Remove eln widgets #608

Closed danielhollas closed 1 month ago

danielhollas commented 1 month ago

Discussed briefly in #595. We could make eln into an optional dependency, but honestly I don't know if it makes sense for it to be in AWB. It does not directly interact with any other widgets and is completely standalone so it would make more sense to me to move the code elsewhere (perhaps into aiidalab-eln package itself) and publish it as a separate app in aiidalab app registry. This is especially true given that we want to make AWB into a library and remove it from the app registry eventually.

@yakutovicha WDYT? Also briefly discussed with @edan-bainglass in last meeting.

This will unblock publishing AWB on conda-forge and integrating it into docker image.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.19%. Comparing base (7c9a3ef) to head (801f746). Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #608 +/- ## ========================================== - Coverage 96.21% 96.19% -0.02% ========================================== Files 11 10 -1 Lines 1188 1105 -83 ========================================== - Hits 1143 1063 -80 + Misses 45 42 -3 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/608/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `96.19% <ø> (-0.02%)` | :arrow_down: | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `96.19% <ø> (-0.02%)` | :arrow_down: | 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.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielhollas commented 1 month ago

Thank you for the context @edan-bainglass, that's very helpful. So are you saying that these widgets should stay in AWB? I am happy to open an alternative PR which only makes them optional.

edan-bainglass commented 1 month ago

Correct, though somewhat hopeful. We'll have to see how it works, but for now, yes, I would say they should stay. I second their optionality.

danielhollas commented 1 month ago

Thanks @edan-bainglass, I've created #609 and am closing this one.

yakutovicha commented 1 month ago

Thanks, @edan-bainglass for explaining. I agree that the ELN support should stay here for the moment. But I would opt to remove it in 3.0.0 and place it either as a separate app or part of the home app, as proposed by @danielhollas.