-
Notifications
You must be signed in to change notification settings - Fork 104
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
feat(edit): add retain
to Array
, Table
and InlineTable
#576
feat(edit): add retain
to Array
, Table
and InlineTable
#576
Conversation
@epage I tried adding it to |
959c0aa
to
b8ab23f
Compare
@epage I applied all changes and force-pushed accordingly. Can I take the opportunity to ask two side-questions?
(Trying to learn how to behave in OSS without annoying anyone) |
pub fn retain<F>(&mut self, mut keep: F) | ||
where | ||
F: FnMut(&Value) -> bool, | ||
{ | ||
self.values | ||
.retain(|item| item.as_value().map(&mut keep).unwrap_or(false)); | ||
} |
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.
This one should be using as_table
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.
Applied
- /// In other words, remove all values for which `keep(&value)` returns `false`.
+ /// In other words, remove all tables for which `keep(&table)` returns `false`.
///
/// This method operates in place, visiting each element exactly once in the
/// original order, and preserves the order of the retained elements.
pub fn retain<F>(&mut self, mut keep: F)
where
- F: FnMut(&Value) -> bool,
+ F: FnMut(&Table) -> bool,
{
self.values
- .retain(|item| item.as_value().map(&mut keep).unwrap_or(false));
+ .retain(|item| item.as_table().map(&mut keep).unwrap_or(false));
crates/toml_edit/src/table.rs
Outdated
pub fn retain<F>(&mut self, mut keep: F) | ||
where | ||
F: FnMut(&str, &mut Value) -> bool, | ||
{ | ||
self.items.retain(|key, item| { | ||
item.value | ||
.as_value_mut() | ||
.map(|value| keep(key, value)) | ||
.unwrap_or(false) | ||
}); |
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.
This one should be dealing with Item
, not Value
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.
Applied
- /// In other words, remove all pairs `(key, value)` for which
- /// `keep(&key, &mut value)` returns `false`.
+ /// In other words, remove all pairs `(key, item)` for which
+ /// `keep(&key, &mut item)` returns `false`.
///
/// The elements are visited in iteration order.
pub fn retain<F>(&mut self, mut keep: F)
where
- F: FnMut(&str, &mut Value) -> bool,
+ F: FnMut(&str, &mut Item) -> bool,
{
- self.items.retain(|key, item| {
- item.value
- .as_value_mut()
- .map(|value| keep(key, value))
- .unwrap_or(false)
- });
+ self.items
+ .retain(|key, key_value| keep(key, &mut key_value.value));
This got a bit weird .retain(|key, key_value| ... key_value.value)
, because this Table maps key -> key_value
.
If its obvious, go ahead. If its not, its nice to leave it open so we can both better track whether a change actually resolved it.
I very much appreciate force-pushes |
added to: `Table`, `InlineTable`, `Array` and `ArrayOfTables`
b8ab23f
to
4b2d3c9
Compare
retain
to Array
, Table
and InlineTable
retain
to Array
, Table
and InlineTable
Thanks! |
Solves #311.