-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fix verdi devel check-undesired-imports when tui extra is installed #6693
base: main
Are you sure you want to change the base?
Fix verdi devel check-undesired-imports when tui extra is installed #6693
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6693 +/- ##
==========================================
- Coverage 78.05% 78.05% -0.00%
==========================================
Files 563 563
Lines 41790 41793 +3
==========================================
Hits 32616 32616
- Misses 9174 9177 +3 ☔ View full report in Codecov by Sentry. |
1b1a850
to
eb1e91b
Compare
@@ -129,8 +129,6 @@ jobs: | |||
with: | |||
python-version: '3.12' | |||
from-lock: 'true' | |||
# NOTE: The `verdi devel check-undesired-imports` fails if | |||
# the 'tui' extra is installed. | |||
extras: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include the extras here again? We just removed the extras because of the tui problem right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the extras were never included in this check, and I don't see why we should include them if they are not needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They caught the error that this PR is fixing or am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I just would like to test it with all extras. We could also make a dedicated tests that adds all dummy modules under the name of the extras into the sys but that would not consider dependencies. I am not sure if there is a smart way to test the fix here without installing all extras.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They caught the error that this PR is fixing or am I wrong?
I wouldn't really call this an error, it is just fixing a test essentially. (note that even though it's part of verdi devel
it's not really meant to be used by users)
I dunno, you're basically asking to test the test. I could argue in the opposite direction -- basically all the CI jobs are installing the extras so it's good to have this one thing without the extras.
Anyway, feel free to push to this branch and merge, I will not have time to get to this this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't really call this an error, it is just fixing a test essentially. (note that even though it's part of verdi devel it's not really meant to be used by users)
ok fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the fix! had just one question
45c2d0a
to
ae9b298
Compare
ae9b298
to
3e83606
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work!
@agoscinski thanks for the review! (btw: just in case you're not aware I don't have permissions to merge things on this repo so somebody else needs to push the button :-) ) |
Fixes #6691.
To test the fix run
This will fail on main with