fix: Preserve dotted-key ordering #808
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| use crate::{InlineTable, Item, Key, Table, Value}; | ||
| /// a place for helper methods supporting the table-like impls | ||
|
|
||
| use crate::table::KeyValuePairs; | ||
|
|
||
| /// GetTableValues provides the logic for displaying a table's items in their parsed order | ||
| pub(crate) trait GetTableValues { | ||
|
Comment on lines
+4
to
+7
There was a problem hiding this comment. It looks like this is a roundabout way to share code for manipulating imo if I were to do this, I would
There was a problem hiding this comment. Feel free to split this refactor out into its own commit. It'll likely make it easier to review and get merged which can speed up reviewing both PRs. Also, please consider if there are intermediate steps to the above that should be broken out into their own commit (e.g. adding a newtype of we go that route) |
||
| fn items(&self) -> &KeyValuePairs; | ||
|
|
||
| fn get_values(&self) -> Vec<(Vec<&Key>, &Value)> { | ||
| let mut values = Vec::new(); | ||
| let root = Vec::new(); | ||
| self.append_values(&root, &mut values); | ||
| values | ||
| } | ||
|
|
||
| fn append_values<'s>( | ||
| &'s self, | ||
| parent: &[&'s Key], | ||
| values: &mut Vec<(Vec<&'s Key>, &'s Value)>, | ||
| ) { | ||
| for (key, item) in self.items().iter() { | ||
| let mut path = parent.to_vec(); | ||
| path.push(key); | ||
| match item { | ||
| Item::Table(table) if table.is_dotted() => { | ||
| GetTableValues::append_values(table, &path, values) | ||
| } | ||
| Item::Value(Value::InlineTable(table)) if table.is_dotted() => { | ||
| GetTableValues::append_values(table, &path, values) | ||
| } | ||
| Item::Value(value) => { | ||
| values.push((path, value)) | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// SortTable provides the logic for sorting a table's items by its keys | ||
| pub(crate) trait SortTable { | ||
| fn items_mut(&mut self) -> &mut KeyValuePairs; | ||
|
|
||
| fn sort_values(&mut self) { | ||
| // Assuming standard tables have their doc_position set and this won't negatively impact them | ||
| self.items_mut().sort_keys(); | ||
| assign_sequential_key_positions(self.items_mut(), |item| { | ||
| match item { | ||
| Item::Table(table) if table.is_dotted() => { | ||
| SortTable::sort_values(table) | ||
| } | ||
| Item::Value(Value::InlineTable(table)) if table.is_dotted() => { | ||
| SortTable::sort_values(table) | ||
| } | ||
| _ => {} | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /// SortTableBy provides the logic for sorting a table by a custom comparison | ||
| pub(crate) trait SortTableBy<It> : SortTable | ||
| where | ||
| It: for<'a> TryFrom<&'a Item> | ||
| { | ||
| fn sort_values_by<F>(&mut self, compare: F) | ||
| where | ||
| F: FnMut(&Key, &It, &Key, &It) -> std::cmp::Ordering, | ||
| { | ||
| // intended for `InlineTable`s, where some `Item`s might not be `Value`s, | ||
| // in the case of dotted keys mostly I expect. | ||
| // but for `Table`s the `(Some,Some)` will be the only one used. | ||
| self.sort_vals_by_direct( | ||
| &mut Self::generalize(compare) | ||
| ) | ||
| } | ||
|
|
||
| /// no modification to the comparing Fn in this one, | ||
| /// allows for slightly improved recursion that does not continuously | ||
| /// re-modify the comparison function. | ||
| fn sort_vals_by_direct<F>(&mut self, compare: &mut F) | ||
| where | ||
| F: FnMut(&Key, &Item, &Key, &Item) -> std::cmp::Ordering | ||
| { | ||
| self.items_mut().sort_by(|key1, val1, key2, val2| { | ||
| compare(key1, val1, key2, val2) | ||
| }); | ||
|
|
||
| assign_sequential_key_positions(self.items_mut(), |value| { | ||
| match value { | ||
| Item::Table(table) if table.is_dotted() => { | ||
| SortTableBy::<Item>::sort_values_by( | ||
| table, | ||
| |k1, i1, k2, i2| { | ||
| compare(k1, i1, k2, i2) | ||
| } | ||
| ) | ||
| }, | ||
| Item::Value(Value::InlineTable(table)) if table.is_dotted() => { | ||
| SortTableBy::<Value>::sort_values_by( | ||
| table, | ||
| |k1, i1, k2, i2| { | ||
| let s1 = &Item::from(i1); | ||
| let s2 = &Item::from(i2); | ||
| compare(k1, s1, k2, s2) | ||
| } | ||
| ) | ||
| }, | ||
| _ => {} | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| fn generalize<'a, F>(mut compare: F) -> Box<dyn FnMut(&Key, &Item, &Key, &Item) -> std::cmp::Ordering + 'a> | ||
| where | ||
| F: FnMut(&Key, &It, &Key, &It) -> std::cmp::Ordering + 'a, | ||
| { | ||
| Box::new(move |key1, s1, key2, s2| { | ||
| match (It::try_from(s1).ok(), It::try_from(s2).ok()) { | ||
| (Some(v1), Some(v2)) => compare(key1, &v1, key2, &v2), | ||
| (Some(_), None) => std::cmp::Ordering::Greater, | ||
| (None, Some(_)) => std::cmp::Ordering::Less, | ||
| (None, None) => std::cmp::Ordering::Equal, | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| fn assign_sequential_key_positions<F>(items: &mut KeyValuePairs, mut recursive_step: F) | ||
| where | ||
| F: FnMut(&mut Item), | ||
| { | ||
| use indexmap::map::MutableKeys; | ||
| for (pos, (key, value)) in items.iter_mut2().enumerate() { | ||
| key.set_position(Some(pos)); | ||
| recursive_step(value); | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<&Item> for Value { | ||
| type Error = String; | ||
|
|
||
| fn try_from(value: &Item) -> Result<Self, Self::Error> { | ||
| let err = "cannot extract Value from Non-Value Item:"; | ||
| match value { | ||
| Item::Value(v) => Ok((*v).clone()), | ||
| it => it.as_value().map(|v| v.clone()).ok_or( | ||
| format!("{err}: {it:?}") | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| impl GetTableValues for Table { | ||
| fn items(&self) -> &KeyValuePairs { | ||
| &self.items | ||
| } | ||
| } | ||
| impl GetTableValues for InlineTable { | ||
| fn items(&self) -> &KeyValuePairs { | ||
| &self.items | ||
| } | ||
| } | ||
|
|
||
| impl SortTable for Table { | ||
| fn items_mut(&mut self) -> &mut KeyValuePairs { | ||
| &mut self.items | ||
| } | ||
| } | ||
| impl SortTable for InlineTable { | ||
| fn items_mut(&mut self) -> &mut KeyValuePairs { | ||
| &mut self.items | ||
| } | ||
| } | ||
|
|
||
| impl SortTableBy<Item> for Table {} | ||
| impl SortTableBy<Value> for InlineTable {} | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
TableandInlineTablehave the semantics of anIndexMap. We need to preserve the order of content as it gets inserted. We could set a position on insertion.Also, I'm unsure about allowing key positions to be moved between tables because the position is dependent on the other values within the same table and you can't easily translate that from one table to the next.
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.
I think I missed this when I was initially reading your feedback, I did not investigate this as an option yet, I'll take a look at this in the next week or so when I can.