pixi icon indicating copy to clipboard operation
pixi copied to clipboard

refactor: feature names inside double quotes if containing a `.`

Open mrswastik-robot opened this issue 7 months ago • 4 comments

fixes #3171

Code refactored is in table_name.rs:

if feature_str.contains('.') {
    parts.push(format!("\"{}\"", feature_str).leak());
} else {
    parts.push(feature_str);
}

Had to use leak() otherwise I was facing lifetime error. Also added a test case, it's passing.

mrswastik-robot avatar Apr 14 '25 20:04 mrswastik-robot

It doesn't work with pixi add -f test.test bat as that will result in:

[feature.'"test'.'test"'.dependencies]
bat = "*"

ruben-arts avatar Apr 15 '25 11:04 ruben-arts

It doesn't work with pixi add -f test.test bat as that will result in:

[feature.'"test'.'test"'.dependencies]
bat = "*"

sorry, @ruben-arts, my bad! is it because the quotes are being included in the string rather than being treated as delimiters? Using format!("\"{}\"", feature_str) inserts literal quotes into the string, but TOML treats them differently... I guess the quoting is being handled by the toml_edit library

mrswastik-robot avatar Apr 15 '25 13:04 mrswastik-robot

Maybe for simplicity and reuse we could just have function or struct to escape a string? I assume there are more reasons to add quotes to keys?

Maybe for simplicity and reuse we could just have function or struct to escape a string? I assume there are more reasons to add quotes to keys?

Maybe for simplicity and reuse we could just have function or struct to escape a string? I assume there are more reasons to add quotes to keys?

yup I get the point, so should I write a generalised function like escape_toml_keys perhaps in a file like crates/pixi_manifest/src/utils/toml_utils.rs, that should work for characters that require esaping in TOML keys?

mrswastik-robot avatar Apr 21 '25 12:04 mrswastik-robot

Makes sense!

baszalmstra avatar Apr 21 '25 15:04 baszalmstra

pixi add -f test.test bat

Still results in:

[feature.'"test'.'test"'.dependencies]
bat = "*"

It would be good to add a simple integration test to figure out that the manifest can still be read after the pixi add command.

ruben-arts avatar May 02 '25 12:05 ruben-arts

hi @nichmor @ruben-arts, I think I found the issue here. Integration test revealed that the current solution still results in [feature.'"test'.'test"'.dependencies], and ig that's mostly because of the manual escaping I have been doing through the escape_toml_key() function inside toml_utils.rs. The problem is that the function is getting applied too early in the pipeline, here's what happening:

// In table_name.rs
if !feature_name.is_default() {
    parts.push("feature");
    let feature_str = feature_name.as_str();
    let escaped_feature = escape_toml_key(feature_str); // ← Applied escaping here
    parts.push(escaped_feature); // ← Now contains "test.test" 
}

// Later in the pipeline, when creating TOML:
let table_name = parts.join("."); // ← "feature.\"test.test\".dependencies"
// Then split again by dots for TOML library:
let parts: Vec<&str> = table_name.split('.').collect(); 
// Result: ["feature", "\"test", "test\"", "dependencies"] ← thus broken result here

so, this results in double escaping...ig the solution is to let the TOML library handle the escaping by keeping parts separated, an example would be like this:

// WRONG: Escaping too early, then splitting later
"feature.\"test.test\".dependencies" → split('.') → ["feature", "\"test", "test\"", "dependencies"]

// RIGHT: Keep parts separate, let TOML library escape
["feature", "test.test", "dependencies"] → TOML library → [feature."test.test".dependencies]

Would love to hear your feedback on this.

mrswastik-robot avatar Jun 08 '25 22:06 mrswastik-robot

Letting the TOML library handle it sounds great!

baszalmstra avatar Jun 09 '25 07:06 baszalmstra

hey @mrswastik-robot ! I checked out your branch, and if I do cargo run -- add -f test.test rich in examples/pypi/pixi.toml

and then

graf@Nichitas-MacBook-Pro pypi % cargo run -- run python
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.99s
     Running `/Users/graf/projects/oss/pixi-build-branch/target/debug/pixi run python`
Error:   × Unexpected keys, expected only 'platforms', 'channels', 'channel-priority', 'target', 'dependencies', 'host-dependencies', 'build-
  │ dependencies', 'pypi-dependencies', 'activation', 'tasks', 'pypi-options', 'system-requirements'
    ╭─[/Users/graf/projects/oss/pixi-build-branch/examples/pypi/pixi.toml:38:19]
 37 │ 
 38 │ [feature.'"test'.'test"'.dependencies]
    ·                   ──┬──
    ·                     ╰── 'test"' was not expected here
 39 │ rich = "*"
    ╰────

I still have the error. Is it expected?

nichmor avatar Jun 10 '25 07:06 nichmor

hey @mrswastik-robot ! I checked out your branch, and if I do cargo run -- add -f test.test rich in examples/pypi/pixi.toml

and then

graf@Nichitas-MacBook-Pro pypi % cargo run -- run python
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.99s
     Running `/Users/graf/projects/oss/pixi-build-branch/target/debug/pixi run python`
Error:   × Unexpected keys, expected only 'platforms', 'channels', 'channel-priority', 'target', 'dependencies', 'host-dependencies', 'build-
  │ dependencies', 'pypi-dependencies', 'activation', 'tasks', 'pypi-options', 'system-requirements'
    ╭─[/Users/graf/projects/oss/pixi-build-branch/examples/pypi/pixi.toml:38:19]
 37 │ 
 38 │ [feature.'"test'.'test"'.dependencies]
    ·                   ──┬──
    ·                     ╰── 'test"' was not expected here
 39 │ rich = "*"
    ╰────

I still have the error. Is it expected?

yes, this was expected, I haven't pushed the solution yet, I will push it as soon as it's ready...the approach I am following is to let the TOML library handle the escaping

mrswastik-robot avatar Jun 10 '25 07:06 mrswastik-robot

https://github.com/prefix-dev/pixi/issues/3171 has already been fixed

baszalmstra avatar Nov 21 '25 14:11 baszalmstra