feat: add pixi global expose command #2030
Open
+742
−316
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…com/nichmor/pixi into feat/add-pixi-global-expose-command
Hofer-Julian
changed the title
baszalmstra
reviewed
Sep 24, 2024
Comment on lines
+60
to
+74
| pub fn add_exposed_mapping( | ||
| &mut self, | ||
| env_name: &EnvironmentName, | ||
| mapping: &Mapping, | ||
| ) -> miette::Result<()> { | ||
| self.document | ||
| .get_or_insert_nested_table(&format!("envs.{env_name}.exposed"))? | ||
| .insert( | ||
| &mapping.exposed_name.to_string(), | ||
| Item::Value(toml_edit::Value::from(mapping.executable_name.clone())), | ||
| ); | ||
|
|
||
| tracing::debug!("Added exposed mapping {mapping} to toml document"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This function brings this object in an inconsistent state where the TOML is updated but the parsed value is not. I consider this bad practice because after this operation is performed it is unclear whether a next operation will use the updated values or not.
We talked about it being ok to use this approach but I think in that case it should also be reflected in the types. For example by converting this type into a TomlManifest (dropping the parsed) and only adding this method on the self.document type.
| @@ -373,23 +389,29 @@ mod tests { | |||
| python = "python" | |||
There was a problem hiding this comment.
Can we change this simple manifest to point to a local channel? That way the tests will be significantly faster.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some missing things:
it is also should be merged after #1975 lands first