New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Record::get
instead of Value
functions
#10925
Conversation
Record::get
instead of Value
cell path functionsRecord::get
instead of Value
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Gotta watch out for potential merge conflicts but we should be moving along with the record stuff for this release
@@ -105,27 +105,24 @@ fn to_md( | |||
} | |||
|
|||
fn fragment(input: Value, pretty: bool, config: &Config) -> String { | |||
let headers = input.columns(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good riddance Value::columns()
is the weirdest method possible.
let group_key = if let Value::Record { val: row, .. } = row { | ||
row.get(&column_name.item) | ||
} else { | ||
None | ||
}; | ||
|
||
match group_key { | ||
Some(group_key) => Ok(group_key.as_string()?), | ||
None => Err(ShellError::CantFindColumn { | ||
col_name: column_name.item.to_string(), | ||
span: column_name.span, | ||
src_span: row.span(), | ||
}), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that this is like the only place where things get a tad less readable.
None, | ||
Vec::new(), | ||
) | ||
} | ||
}; | ||
|
||
let span = match label.get_data_by_key("span") { | ||
Some(val @ Value::Record { .. }) => val, | ||
let (span, span_span) = match label.get("span") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo I heard you like span so I put a span next to your span 😆
# Description This is pretty complementary/orthogonal to @IanManske 's changes to `Value` cellpath accessors in: - #10925 - to a lesser extent #10926 ## Steps - Use `R.remove` in `Value.remove_data_at_cell_path` - Pretty sound after #10875 (tests mentioned in commit message have been removed by that) - Update `did_you_mean` helper to use iterator - Change `Value::columns` to return iterator - This is not a place of honor - Use `Record::get` in `Value::get_data_by_key` # User-Facing Changes None intentional, potential edge cases on duplicated columns could change (considered undefined behavior) # Tests + Formatting (-)
# Description Where appropriate, this PR replaces instances of `Value::get_data_by_key` and `Value::follow_cell_path` with `Record::get`. This avoids some unnecessary clones and simplifies the code in some places.
# Description This is pretty complementary/orthogonal to @IanManske 's changes to `Value` cellpath accessors in: - nushell#10925 - to a lesser extent nushell#10926 ## Steps - Use `R.remove` in `Value.remove_data_at_cell_path` - Pretty sound after nushell#10875 (tests mentioned in commit message have been removed by that) - Update `did_you_mean` helper to use iterator - Change `Value::columns` to return iterator - This is not a place of honor - Use `Record::get` in `Value::get_data_by_key` # User-Facing Changes None intentional, potential edge cases on duplicated columns could change (considered undefined behavior) # Tests + Formatting (-)
# Description Where appropriate, this PR replaces instances of `Value::get_data_by_key` and `Value::follow_cell_path` with `Record::get`. This avoids some unnecessary clones and simplifies the code in some places.
# Description This is pretty complementary/orthogonal to @IanManske 's changes to `Value` cellpath accessors in: - nushell#10925 - to a lesser extent nushell#10926 ## Steps - Use `R.remove` in `Value.remove_data_at_cell_path` - Pretty sound after nushell#10875 (tests mentioned in commit message have been removed by that) - Update `did_you_mean` helper to use iterator - Change `Value::columns` to return iterator - This is not a place of honor - Use `Record::get` in `Value::get_data_by_key` # User-Facing Changes None intentional, potential edge cases on duplicated columns could change (considered undefined behavior) # Tests + Formatting (-)
Description
Where appropriate, this PR replaces instances of
Value::get_data_by_key
andValue::follow_cell_path
withRecord::get
. This avoids some unnecessary clones and simplifies the code in some places.