Skip to content
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

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

Klaim
Copy link
Member

@Klaim Klaim commented Mar 14, 2022

Adds the handling of environment lockfile (*-lock.yaml files) specifying specific dependency versions (see conda lock) when using the subcommands create and install.

@Klaim Klaim changed the title lockfile parsing environment lockfile parsing Mar 14, 2022
@Klaim Klaim force-pushed the klaim/env-lockfile branch 3 times, most recently from c5a384f to 34eeb4d Compare March 15, 2022 14:24
@Klaim Klaim force-pushed the klaim/env-lockfile branch 3 times, most recently from ba660a3 to 3863558 Compare March 25, 2022 11:29
@Klaim Klaim changed the title environment lockfile parsing environment lockfile parsing & usage in create and install sub-commands Mar 25, 2022
@Klaim Klaim marked this pull request as ready for review March 25, 2022 13:25
@Klaim
Copy link
Member Author

Klaim commented Mar 25, 2022

There is one test (or set of tests) missing that I'm working on, but it could be added after.

@Klaim Klaim force-pushed the klaim/env-lockfile branch 2 times, most recently from 30bc3fb to 4cab1b8 Compare March 25, 2022 14:01
/// invalid values for our purpose.
};

class EnvLockFileError : public mamba_error
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@Klaim Klaim force-pushed the klaim/env-lockfile branch 2 times, most recently from 3f86dc3 to 3d45f9d Compare March 28, 2022 09:44
@Klaim
Copy link
Member Author

Klaim commented Mar 28, 2022

After discussion with Johan I will remove the additional error type and move it's data into our mamba_error type. Wait for it before merging ^^

@Klaim Klaim force-pushed the klaim/env-lockfile branch from 6bef858 to ca7311b Compare March 28, 2022 11:07
@Klaim
Copy link
Member Author

Klaim commented Mar 28, 2022

@JohanMabille I did the changes we talked about regarding error handling, is it fine this way?

@JohanMabille
Copy link
Member

Yes, that's perfect!

@JohanMabille
Copy link
Member

@wolfv I think this one can be merged.

@Klaim
Copy link
Member Author

Klaim commented Mar 29, 2022

Agreed. I will put the tests in another PR once I fix the issue that this PR introduces with --json (basically it seems the json output is malformed but only with lockfiles, I see why but didn´t find a good solution yet). That issues prevents me from doing the checks in the tests.

@Klaim Klaim mentioned this pull request Mar 29, 2022
Klaim added 2 commits April 7, 2022 12:00
- add lockfile parsing functions with tests
- integrate lockfile detection and reading into `create` and `install`
@Klaim Klaim force-pushed the klaim/env-lockfile branch from 08181ba to 22d91d3 Compare April 7, 2022 10:13
@Klaim
Copy link
Member Author

Klaim commented Apr 7, 2022

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>();
Copy link
Member

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?

Copy link
Member Author

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.

@JohanMabille JohanMabille merged commit 5c41be2 into mamba-org:master Apr 8, 2022
@Klaim Klaim deleted the klaim/env-lockfile branch December 19, 2024 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants