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

Add support for getting array index in GetValue #321

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Sep 18, 2023

Add the ability to use an index number to get an item out of a slice.

The number will still be a string representation of an integer. For
example, to get the last element from a nested slice like:

data := map[string]interface{}{
    "data": []interface{}{
        "first",
        "second",
        "third",
    },
}

Use keys like

val := GetValues(data, "dat", "2")

rancher/rancher#42767

Copy link
Contributor

@JonCrowther JonCrowther left a comment

Choose a reason for hiding this comment

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

LGTM, one question for my personal understanding

pkg/data/values_test.go Show resolved Hide resolved
pkg/data/values_test.go Outdated Show resolved Hide resolved
pkg/data/values_test.go Show resolved Hide resolved
@cmurphy cmurphy force-pushed the get-value-arrays branch 6 times, most recently from b470879 to ac940a2 Compare September 22, 2023 22:25
pkg/data/values.go Outdated Show resolved Hide resolved
Add the ability to use an index number to get an item out of a slice.

The number will still be a string representation of an integer. For
example, to get the last element from a nested slice like:

```
data := map[string]interface{}{
    "data": []interface{}{
        "first",
        "second",
        "third",
    },
}
```

Use keys like

```
val := GetValues(data, "data", "2")
```
Copy link

@maxsokolovsky maxsokolovsky left a comment

Choose a reason for hiding this comment

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

A few doc suggestions and additional test cases.

pkg/data/values.go Outdated Show resolved Hide resolved
pkg/data/values_test.go Outdated Show resolved Hide resolved
pkg/data/values.go Outdated Show resolved Hide resolved
pkg/data/values.go Outdated Show resolved Hide resolved
pkg/data/values.go Outdated Show resolved Hide resolved
Adds package comments and extra test cases
maxsokolovsky
maxsokolovsky previously approved these changes Feb 6, 2024
@MbolotSuse MbolotSuse requested review from tomleb and removed request for KevinJoiner February 13, 2024 21:24
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

@tomleb pointed out that although the function is technically backward compatible, the signature no longer is. To make this backward compatible and not force a release of wrangler v3.0, we will need to keep the interface compatible. I'll do this by making the current GetValue in this PR a new function, and keeping the original function consistent.

// For a map, a key denotes the key in the map whose value we want to retrieve.
// For the slice, it denotes the index (starting at 0) of the value we want to retrieve.
// Returns the retrieved value (if any) and a bool indicating if the value was found.
func GetValueFromAny(data interface{}, keys ...string) (interface{}, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder whether it would be a good idea to allows keys to be any and validate that they're string or a number..
This way we could be sure that -1 passed in expected a list vs "-1" which would expect a map.

This does add more complexity with this function like what to return if you receive an unexpected type, etc. So I think it's okay as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the way this function is used in Steve is that keys are essentially user provided input. So if we wen that direction Steve would need to do some selective parsing there, which I don't think is desirable.

That being said, I think that you are correct that the underlying library really should be more specific than this. I tried to implement a more comprehensive approach that (IIRC) did what you suggested in #325 (though I doubt that will merge). I'd say if we want an improvement like that we should consider a rewrite like that Pr looks for.

Restores GetValue to it's original interface to avoid breaking changes.
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

LGTM. I think we could add tests for the GetValue function now that it's not relying on GetValueFromAny, but up to you.

@MbolotSuse MbolotSuse dismissed maxsokolovsky’s stale review April 9, 2024 19:00

Stale, based on old changes.

@MbolotSuse MbolotSuse merged commit 5527d17 into rancher:master Apr 9, 2024
2 checks passed
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

6 participants