feat: Add to_path() method to ExplicitEnvironmentSpec

[synapticarbors]

Based on feedback from @baszalmstra on discord, I added a to_path method to the ExplicitEnvironmentSpec struct so that it could be dumped to file. This would be a first step in a pixi export PR to be submitted later.

This is my first rust-based PR to a project (although I've fooled around with it a bit on my own), so feedback is very welcome. For testing, ExplicitEnvironmentSpec and ExplicitEnvironmentEntry didn't implement PartialEq, so I just unrolled the content. This isn't ideal, but I didn't want to start poking around on the main struct definitions, but I can if that's preferred.

[baszalmstra]

Maybe only write this part if the platform is specified.

E.g.

Suggested change

	out.push_str(&format!("# platform: {plat}\n"));
	if let Some(platf) = &self.platform {
	out.push_str(&format!("# platform: {plat}\n"));
	}

[baszalmstra]

Im not sure if this part should be included by default. E.g. you could also use (micro)mamba for instance.

[baszalmstra]

Suggested change

	for p in self.packages.iter() {
	for p in &self.packages {

[baszalmstra]

No need, this happens on drop.

Suggested change

tmp_dir.close().unwrap()

[baszalmstra]

Suggested change

	assert_matches!(env.to_path(file_path.clone()), Ok(()));
	env.to_path(file_path.clone()).expect("failed to write to path!")

[synapticarbors]

I just realized I missed this one suggestion in the tests. Do you want me to open another PR?

[baszalmstra]

Its fine, dont worry about it! 👍

Thanks again for contributing!

[baszalmstra]

Might also be interesting to implement this (still draft) spec completely: conda/ceps#79

[synapticarbors]

@baszalmstra -- Thank you for the quick review. I'll make the code changes later today. I did have a few questions:

Might also be interesting to implement this (still draft) spec completely: conda/ceps#79

I read through the CEP and was trying to figure out what was needed to implement this completely. The only ideas that I had were arguments to the proposed to_string method like to_string(&self, include_platform: bool, comments: Option<String>). Those some arguments would be passed to the to_path method and then would be passed down to to_string. Let me know if that sounds reasonable or if you prefer a different way or signature.

The other requirements in the CEP were:

• Topologically sorted url list: This seems outside of the scope of this struct as it has no information or knowledge about package inter-dependencies
• Expanding paths and no environment variables: I wasn't sure about this as the Url type in rust seems to be "absolute" and will error on relative file paths when using from_path() for example.

Please let me know what in the CEP is missing in your mind and I'll try to implement it.

[baszalmstra]

I hadnt really checked what needed to happen to implement the CEP. 😅 I think most of it is related to how we parse the file not so much how we write it. In that sense I think your implementation already contains everything required.

I dont think it makes sense to add a comments argument to to_string. Users can just call the to_string method and prepend whatever if they really want to. The platform is also not really needed because you could just as well set it to None in the struct itself.

[synapticarbors]

@baszalmstra -- So it sounds like what you think is the best approach is:

1. Add to_string(). This should take the ExplicitEnvironmentSpec and return String containing:

"# platform: linux-64\n@EXPLICIT\nURL_1\nURL_2\n...."

if Some(plat), else:

"@EXPLICIT\nURL_1\nURL_2\n...."

2. Modify my current to_path to use to_string(). I guess here maybe we'd still want comments: Option<String> to allow custom comments to be written to the file? Or just basically write the output of to_string() to file and leave it to the user to write their own custom function if they want any prepended comments.

[baszalmstra]

Indeed. And I would indeed forgo the comments for now. If people want that they can write their custom function.

[synapticarbors]

@baszalmstra -- I made the requested changes. I just wanted to note that cargo clippy complains about the to_string method:

❯ cargo clippy
    Checking rattler_conda_types v0.26.1 (.../rattler/crates/rattler_conda_types)
warning: implementation of inherent method `to_string(&self) -> String` for type `explicit_environment_spec::ExplicitEnvironmentSpec`
   --> crates/rattler_conda_types/src/explicit_environment_spec.rs:156:5
    |
156 | /     pub fn to_string(&self) -> String {
157 | |         let mut s = String::new();
158 | |
159 | |         if let Some(plat) = &self.platform {
...   |
169 | |         s
170 | |     }
    | |_____^
    |
    = help: implement trait `Display` for type `explicit_environment_spec::ExplicitEnvironmentSpec` instead
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#inherent_to_string

So maybe an alternative name? Ignore the warning? Or implement Display?. Also, I'm wondering if the to_path() is worth adding since it does so little.

[baszalmstra]

If you can think of a better name be my guest. I would also be fine with suppressing this particular lint.

Even though to_path does little its a nice little utlity function. 👍

[synapticarbors]

The best alternatives I could think of at this point are to_spec_string() and render_spec_string(). You're right that implementing Display doesn't seem correct since what this is outputting is a rendering of the struct to the spec, rather than a general representation of the struct. If you like this alternative, I could also change to_path() -> to_spec_path()/render_spec_path().

[baszalmstra]

I think to_path is still fine and to_spec_string also sounds great!

[synapticarbors]

@baszalmstra I've renamed the method so everything should be in order now.