Skip to content
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

Merged

Conversation

marcospb19
Copy link
Contributor

@marcospb19 marcospb19 commented Jun 28, 2023

Solves #311.

@marcospb19
Copy link
Contributor Author

@epage I tried adding it to TableLike, but the generic for FnMut(...) -> bool made a compilation error, object safety requires that I don't put a generic in the retain method, I believe I could try with &dyn or &mut dyn, not sure if it works tho.

@marcospb19 marcospb19 force-pushed the toml-edit-add-retain-method-to-collections branch from 959c0aa to b8ab23f Compare June 28, 2023 16:15
@marcospb19
Copy link
Contributor Author

marcospb19 commented Jun 28, 2023

@epage I applied all changes and force-pushed accordingly.

Can I take the opportunity to ask two side-questions?

  1. Do you prefer me to mark as resolved, or leave it to the reviewer to resolve?
  2. Is force pushing in this context bad?

(Trying to learn how to behave in OSS without annoying anyone)

Comment on lines 101 to 107
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));
}
Copy link
Member

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

Copy link
Contributor Author

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));

Comment on lines 407 to 416
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)
});
Copy link
Member

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

Copy link
Contributor Author

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.

@epage
Copy link
Member

epage commented Jul 3, 2023

  1. Do you prefer me to mark as resolved, or leave it to the reviewer to resolve?

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.

2. Is force pushing in this context bad?

I very much appreciate force-pushes

added to: `Table`, `InlineTable`, `Array` and `ArrayOfTables`
@marcospb19 marcospb19 force-pushed the toml-edit-add-retain-method-to-collections branch from b8ab23f to 4b2d3c9 Compare July 5, 2023 18:41
@epage epage changed the title toml_edit: add retain to Array, Table and InlineTable feat(edit): add retain to Array, Table and InlineTable Jul 5, 2023
@epage epage merged commit c2a36b0 into toml-rs:main Jul 5, 2023
8 checks passed
@epage
Copy link
Member

epage commented Jul 5, 2023

Thanks!

@marcospb19 marcospb19 deleted the toml-edit-add-retain-method-to-collections branch July 5, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants