maint: YAML environment specification utilities#4158
maint: YAML environment specification utilities#4158jjerphan wants to merge 2 commits intomamba-org:mainfrom
Conversation
jjerphan force-pushed the maint/yaml-utils branch from 70b0bd5 to deb8342 Compare 
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #4158 +/- ## ======================================== Coverage 52.15% 52.15% ======================================== Files 239 241 +2 Lines 28803 29009 +206 Branches 3029 3066 +37 ======================================== + Hits 15021 15131 +110 - Misses 13779 13875 +96 Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jjerphan force-pushed the maint/yaml-utils branch 6 times, most recently from 4f44235 to aae4296 Compare jjerphan force-pushed the maint/yaml-utils branch 2 times, most recently from 6cb440d to e1dc8a2 Compare jjerphan force-pushed the maint/yaml-utils branch from e1dc8a2 to ea88ffd Compare 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan force-pushed the maint/yaml-utils branch from ea88ffd to 6577cdd Compare Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan left a comment
jjerphan left a comment There was a problem hiding this comment.
Some points of discussions.
| try | ||
| { | ||
| const auto& channels = channel_context.make_channel(channel_str); | ||
| if (!channels.empty()) | ||
| { | ||
| return channels.front().id(); | ||
| } | ||
| } | ||
| catch (...) | ||
| { | ||
| // If resolution fails, try to extract channel name from URL | ||
| // e.g., "https://conda.anaconda.org/conda-forge" -> "conda-forge" | ||
| if (channel_str.find("conda.anaconda.org/") != std::string::npos) | ||
| { | ||
| auto parts = util::rsplit(channel_str, '/'); | ||
| if (parts.size() >= 2) | ||
| { | ||
| return parts[parts.size() - 1]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We could benefit from using expected here, but this would be yet another refactor which would impact other files.
What is the best trade-off? Should we open another PR?
There was a problem hiding this comment.
I don't think that would help with channel_context.make_channel as it is used everywhere as a getter for channel data and it do need to throw as any failure with that call would be a failure.
I prefer to use expected when the error is part of the algorithm, for example as return type for parsing functions (there are other examples).
What I'm not sure of is why is there a try-catch here? "resolution" isnt supposed to happen in make_channel, so I dont get the meaning of the comment in the catch.
Is that why you were thinking of using expected?
| // Helper function: Convert PackageInfo to MatchSpec string | ||
| std::string package_to_spec_string( | ||
| const specs::PackageInfo& pkg, | ||
| ChannelContext& channel_context, | ||
| bool no_builds = false, | ||
| bool ignore_channels = false, | ||
| bool include_md5 = false | ||
| ) |
There was a problem hiding this comment.
This could be a method on PackageInfo, but it is only used here and there already are similar free functions; so I am not sure that this is worth it.
There was a problem hiding this comment.
Are changes made to this file API breakages?
| catch (nlohmann::json::exception&) | ||
| { | ||
| // If parsing fails, leave variables empty | ||
| } |
There was a problem hiding this comment.
Should we warn instead here?
2 participants
Description
Per-requisite for #4153.
Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.