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

Move to App widget and call from notebook #442

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Move to App widget and call from notebook #442

merged 3 commits into from
Aug 29, 2023

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Aug 18, 2023

For the test purpose of #428 proposed by @superstar54, we think moving the code from qe.ipynb into a module and creating an App class would make it not only more maintainable and easy for integrate unit test of the whole app as an widget.

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Patch coverage: 30.90% and project coverage change: -0.62% ⚠️

Comparison is base (bd7f39b) 56.52% compared to head (64232c0) 55.91%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
- Coverage   56.52%   55.91%   -0.62%     
==========================================
  Files          26       27       +1     
  Lines        2321     2375      +54     
==========================================
+ Hits         1312     1328      +16     
- Misses       1009     1047      +38     
Flag Coverage Δ
python-3.10 55.91% <30.90%> (-0.62%) ⬇️
python-3.8 55.96% <30.90%> (-0.62%) ⬇️
python-3.9 55.96% <30.90%> (-0.62%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/aiidalab_qe/app/main.py 26.92% <26.92%> (ø)
src/aiidalab_qe/app/__init__.py 100.00% <100.00%> (ø)
src/aiidalab_qe/app/common/__init__.py 100.00% <100.00%> (ø)

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

@unkcpz
Copy link
Member Author

unkcpz commented Aug 18, 2023

@superstar54 you mentioned the dlink may not necessary. I think it depends on the scenario and if you can decouple step widgets by adding another "data" layer in between. If you want to make such a change, please make a dedicated PR for that.

src/aiidalab_qe/app/main.py Outdated Show resolved Hide resolved
src/aiidalab_qe/app/__init__.py Outdated Show resolved Hide resolved
@unkcpz unkcpz changed the title Move to MainApp widget and call from notebook Move to App widget and call from notebook Aug 29, 2023
Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

LGTM!

@unkcpz unkcpz merged commit 4bf06f0 into main Aug 29, 2023
12 checks passed
@unkcpz unkcpz deleted the app-module branch August 29, 2023 15:16
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.

None yet

2 participants