-
Notifications
You must be signed in to change notification settings - Fork 11
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
[chore] install for command #181
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
- Coverage 98.23% 98.15% -0.08%
==========================================
Files 9 9
Lines 1077 1086 +9
==========================================
+ Hits 1058 1066 +8
- Misses 19 20 +1 ☔ View full report in Codecov by Sentry. |
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.
Looking good but looks like there's a schema validation error. Should be good once tests pass.
One small comment about testing strategy.
@pytest.mark.parametrize( | ||
"install_args,expected_env", [("", "bbb"), ("--for-command=default", "default")] | ||
) | ||
def test_install_env_for_command(project_directory_factory, install_args, expected_env): |
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.
Looks good, but maybe we should isolate the "determine environment from args" logic into a function and just unit test to speed things up?
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.
Yeah, I'll do that.
The pydantic errors are limited to conda version 24.1 and 24.4. I'm looking into reproducing this locally. |
install an env for a specified command (especially if it is not default)