maint: YAML environment specification utilities#4158
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 53.93% 54.36% +0.43%
==========================================
Files 240 242 +2
Lines 29704 32027 +2323
Branches 3168 3199 +31
==========================================
+ Hits 16022 17413 +1391
- Misses 13679 14611 +932
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
2 times, most recently
from
ea88ffd to
6577cdd
Compare
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?
jjerphan
force-pushed
the
maint/yaml-utils
branch
from
c2cea18 to
ad702bb
Compare
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
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.