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

Allow integer indexing (closes #92) #116

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thorio
Copy link
Contributor

@thorio thorio commented Jul 23, 2024

This PR contains a few related changes for various issues:

@thorio thorio changed the title Allow integer indexing Allow integer indexing (closes #92) Jul 24, 2024
@thorio thorio marked this pull request as ready for review July 27, 2024 09:14
@amircodota
Copy link

@thorio ran a quick test with my use case

Works like a charm for both top level arrays and also arrays in sub-keys 👑

@thorio
Copy link
Contributor Author

thorio commented Aug 24, 2024

@SergioBenitez could you take a look at this?

.gitignore Outdated Show resolved Hide resolved
src/coalesce.rs Outdated
@@ -27,9 +29,10 @@ impl Coalescible for Value {
fn coalesce(self, other: Self, o: Order) -> Self {
use {Value::Dict as D, Value::Array as A, Order::*};
match (self, other, o) {
(D(t, a), D(_, b), Join | Adjoin) | (D(_, a), D(t, b), Merge | Admerge) => D(t, a.coalesce(b, o)),
(D(t, a), D(_, b), Join | Adjoin | Zipjoin) | (D(_, a), D(t, b), Merge | Admerge | Zipmerge) => D(t, a.coalesce(b, o)),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long. Lines should never exceed 100 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the join and merge cases into separate lines, I think this is more readable than any wrapping option.

src/figment.rs Outdated Show resolved Hide resolved
src/figment.rs Outdated Show resolved Hide resolved
src/coalesce.rs Show resolved Hide resolved
src/value/value.rs Outdated Show resolved Hide resolved
Comment on lines 127 to 134
/// "duck" => Value::from(vec![
/// map!{
/// "pasta" => 42usize,
/// },
/// map!{
/// "pizza" => 229usize,
/// },
/// ]),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make one of the other values a vec![]? The smaller this value is, the easier the example is to parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would involve either the array being fairly deeply nested and/or having an array of mixed types. I instead opted to remove one value from each top-level key.

src/value/value.rs Outdated Show resolved Hide resolved
Comment on lines +429 to +432
pub(crate) fn is_none(&self) -> bool {
matches!(self, Value::Empty(_, Empty::None))
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed to remove all of the special-casing.

src/value/value.rs Outdated Show resolved Hide resolved
Round 1.
Once reviewed, these changes should be merged into the appropriate
commits to produce a clean history.
@thorio
Copy link
Contributor Author

thorio commented Aug 31, 2024

@SergioBenitez I fixed all the issues with obvious solutions, I think the rest warrant further discussion.

The big benefit with the zip- strategies is the handling of "there is no value here" values, without it the Env provider would not work correctly for MYARRAY.0=10, MYARRAY.1=20. I picked None for this because it was the simplest option, as well as being fairly pointless in arrays (you'd just remove the entry and shorten the array instead of putting None there).

This also touches on #112, where the user wanted a way to specify "there is no value here", such that None would not replace the previous Some value.

So we need to differentiate between these three cases when merge-ing:

  • "I have an incoming value and I want to replace the existing value" => Some(value)
  • "I do not have an incoming value and want to remove the existing value" => None
  • "I do not have an incoming value and want to leave existing value untouched" => ???

join-ing works the same way, with "incoming value" and "existing value" swapped.

Thus I think we do need something like Value::Void that will always be replaced, regardless of strategy, such that

            [1, 2, _, 4] 
merged with [_, 8, 3, _]
becomes     [1, 8, 3, 4]        where _ is this "void" value.

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.

3 participants