Implementation note:

I'm pretty annoyed by the duplicated if check. I wanted to use an or-pattern (PathMember::Int { val, optional, .. } | PathMember::String { val, optional, .. } =>) but that doesn't work because val has different types in each branch.

Extracting the check into a function or macro also seems overkill for just 2 repetitions.

If anyone has better suggestions, please let me know.

Ok, I forgot to pass --workspace to cargo test. That revealed some failing select tests. I updated them to expect the question marks in the output's column names.

While this looks pretty weird, select can already produce weird column names, e.g. { x: { y: 2 } } | select $.x.y produces { "x.y": 2 }. Now { x: { y: 2 } } | select $.x?.y produces { "x?.y": 2 } instead of { "x.y": 2 }.

However, in (what I assume is) the very common case of selecting a single optional column (select x?), the output column is now named x?. This could be a pretty annoying breaking change. A possible workaround is to use get -i x | wrap x instead.

While this looks pretty weird, select can already produce weird column names, e.g. { x: { y: 2 } } | select $.x.y produces { "x.y": 2 }. Now { x: { y: 2 } } | select $.x?.y produces { "x?.y": 2 } instead of { "x.y": 2 }.

Yeah, discovered this behavior of select generating new column names from the cell-path's when fixing a bug as well

• select renames my columns #13359
• Fix select cell path renaming behavior #13361
• Originally the behavior was added as part of Soft rest arguments column path cohersions. #3016

Apart from that whole thing (mis?)using the Display implementation there is another problem in the representation

• Opened Cell path display output doesn't escape/quote properly #13362 , because column/rows can be indistinguishable and the output gives no strong indication towards it being a cell path (so you can't just copy the output back as a litieral)

Thanks for fixing the optional cell-path part!

@sholderbach does that mean we're ready to land this?

I kind of think we should decouple select's behavior from the changes we plan to make to get a better cell path output.

@aionescu do you want to implement the rest of the improvements to the cell path display output as @IanManske and I discussed in #13362 and #14090 ?

@sholderbach Yeah, they look easy enough (famous last words?). Should I add them all to this PR, or make a separate one?

Should I add them all to this PR, or make a separate one?

Whatever is practical to you, if you want to update the select tests a few times or once. But if it is quicker to get the pieces done as individual PR its fine as well. We only don't want to drag too many breaking changes over multiple releases, but that should be doable.

Thanks

I think it's actually a breaking change:

# Before this PR the following line works:
http get https://api.github.com/repos/nushell/nushell/releases/latest | select -i tag_name assets?.id? | flatten | rename -c {'assets.id': id}
# After this PR the above line will broken and should be changed to:
http get https://api.github.com/repos/nushell/nushell/releases/latest | select -i tag_name assets?.id? | flatten | rename -c {'assets?.id?': id}

It's weird that after this change the column names will have a ? suffix if select with -i, it may be unacceptable?:

[[version name]; ['0.1.0' nushell] ['0.1.1' fish] ['0.2.0' zsh]] | select -i version name | columns
 0   version? 
 1   name? 

Good catch. The column names should be required to have ? for renames and selects even if they're optional. Let's revert.