collect: don't require a closure

[devyn]

Description

This changes the collect command so that it doesn't require a closure. Still allowed, optionally.

Before:

open foo.json | insert foo bar | collect { save -f foo.json }

After:

open foo.json | insert foo bar | collect | save -f foo.json

The closure argument isn't really necessary, as collect values are also supported as PipelineData.

User-Facing Changes

• collect command changed

Tests + Formatting

Example changed to reflect.

After Submitting

• release notes
• we may want to deprecate the closure arg?

[IanManske]

we may want to deprecate the closure arg?

I would be in favor of deprecating the closure arg. We would have to figure out another way to implement the $in variable before that. Although, we might be able to just hide/remove the closure arg in the signature, but still have the parser pass the closure arg to collect.

[WindSoilder]

I don't think it's a breaking change because it converts from a required parameter to optional parameter, so I will remove the label

[WindSoilder]

I think we also need to change parser, it uses collect command to build $in variable (at line 6249).

nushell/crates/nu-parser/src/parser.rs

Lines 6241 to 6255 in 92831d7

	fn wrap_expr_with_collect(working_set: &mut StateWorkingSet, expr: &Expression) -> Expression {
	let span = expr.span;
	if let Some(decl_id) = working_set.find_decl(b"collect") {
	let mut output = vec![];
	let var_id = IN_VARIABLE_ID;
	let mut signature = Signature::new("");
	signature.required_positional.push(PositionalArg {
	var_id: Some(var_id),
	name: "$in".into(),
	desc: String::new(),
	shape: SyntaxShape::Any,
	default_value: None,
	});

[sholderbach]

To deliver on the promise for the open ... | collect | save ... round trip we should strip the metadata if it matches DataSource::FilePath. The others should probably remain if we don't want to commit to this side effect but I would say that DataSource::FilePath is currently an implementation detail for the particular challenge of inplace modifying a file.

[devyn]

Does this make sense? I changed the save command to not check the metadata if it's a Value, because there shouldn't be an issue with reading streams in that case

[sholderbach]

I changed the save command to not check the metadata if it's a Value, because there shouldn't be an issue with reading streams in that case

In my view @IanManske's suggestion is an orthogonal improvement.

I think for this PR on collect we can make stronger guarantees for collect if you have following lazy operations:

open cycle.txt | collect | filter { true } | save cycle.txt

[devyn]

Okay, I think I've fixed it to be correct. File path metadata is removed on collect, and it's also set on the input of the closure rather than the output

Added a couple tests to save that use collect to ensure this works the right way

[sholderbach]

Thanks! Good additional tests