Add CondaURL #2805
Add CondaURL #2805
Conversation
AntoinePrv
force-pushed
the
conda-url
branch
7 times, most recently
from
663f2ba
to
6e3ff05
Compare
AntoinePrv
force-pushed
the
conda-url
branch
from
6e3ff05
to
b49b1ea
Compare
| return static_cast<std::size_t>(Platform::count_); | ||
| } | ||
|
|
||
| constexpr auto known_platforms() -> std::array<Platform, known_platforms_count()>; |
There was a problem hiding this comment.
There is already the notion of known_platforms in channel.hpp, it would make sense to rebind it to your changes (or replace its usage with your), to avoid duplication.
There was a problem hiding this comment.
The point is to use the known_platform from this file from now, on. It's tested , modular, backed by an enum, and constexpr.
There was a problem hiding this comment.
Yes, I meant to use the code from this PR in the channel file; but this can be done in a dedicated PR.
| @@ -57,99 +61,156 @@ namespace mamba::util | |||
| [[nodiscard]] auto scheme() const -> const std::string&; | |||
|
|
|||
| /** Set a non-empty scheme. */ | |||
| auto set_scheme(std::string_view scheme) -> URL&; | |||
| void set_scheme(std::string_view scheme); | |||
There was a problem hiding this comment.
It's annoying to lose the chained construction of the URL. I wonder if it could make sense to use a CRTP here, or even to merge the CondaURL and the URL classes? (do we have usages of URL where we don't want to have access to the additiona methods form CondaURL?
There was a problem hiding this comment.
After discussion in person:
- having two distinct abstractions for URL and CondaURL is better thatn a single class; it helps to keep isolation of the dependency on CURL
- CRTP is a bit of overkill just for keeping the chained construction syntax.
No description provided.