-
Notifications
You must be signed in to change notification settings - Fork 379
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
environment lockfile parsing & usage in create
and install
sub-commands
#1577
Conversation
c5a384f
to
34eeb4d
Compare
ba660a3
to
3863558
Compare
create
and install
sub-commands
There is one test (or set of tests) missing that I'm working on, but it could be added after. |
30bc3fb
to
4cab1b8
Compare
/// invalid values for our purpose. | ||
}; | ||
|
||
class EnvLockFileError : public mamba_error |
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 added a "m_data" member of type any
to mamba_error
so that you can store additional information without having to subclass it. Do you think it would make sense to use it to store the parsing error code?
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 used it to store the YAMLcpp error type_index, but I could store the code there too. This would mean storing a struct instead then.
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 pushed the change, also improved the interface. Storing extra data using this technique with inheriting error types seems to work well.
3f86dc3
to
3d45f9d
Compare
After discussion with Johan I will remove the additional error type and move it's data into our |
6bef858
to
ca7311b
Compare
@JohanMabille I did the changes we talked about regarding error handling, is it fine this way? |
Yes, that's perfect! |
ca7311b
to
08181ba
Compare
@wolfv I think this one can be merged. |
Agreed. I will put the tests in another PR once I fix the issue that this PR introduces with |
- add lockfile parsing functions with tests - integrate lockfile detection and reading into `create` and `install`
08181ba
to
22d91d3
Compare
The issue preventing me to add tests here is fixed (#1600) so I added the tests here. |
/* .platform = */ package_node["platform"].as<std::string>(), | ||
}; | ||
|
||
package.info.version = package_node["version"].as<std::string>(); |
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.
Wouldn't this throw a YAML Exception if no version is available? Just wondering if we should catch the exception and transform to an expected?
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.
So far for any Yamlcpp exception, I assumed it's fine to catch it in read_environment_lockfile
(below) instead of every functions, but I could catch them earlier and return expected indeed. In read_environment_lockfile
the exception is already converted to an expected, though it's not visible from here so no problem if you prefer local catching instead.
Adds the handling of environment lockfile (
*-lock.yaml
files) specifying specific dependency versions (seeconda lock
) when using the subcommandscreate
andinstall
.