Skip to content

Conversation

@marquiz
Copy link
Contributor

@marquiz marquiz commented Aug 21, 2025

No description provided.

@marquiz
Copy link
Contributor Author

marquiz commented Aug 21, 2025

@klihub @chrishenzie @mikebrow

Pre-work for some of my upcoming work on managing the linux.intelRdt object of the runtime-spec. I decided to submit this one as a separate PR with the hopes of easier and more focused review.

I'm not sure if we need all the pointer's-pointer (or nil's-nil) stuff or would it be sufficient to return []string (instead of *[]string) from Get()

@klihub
Copy link
Member

klihub commented Aug 21, 2025

@klihub @chrishenzie @mikebrow

Pre-work for some of my upcoming work on managing the linux.intelRdt object of the runtime-spec. I decided to submit this one as a separate PR with the hopes of easier and more focused review.

I'm not sure if we need all the pointer's-pointer (or nil's-nil) stuff or would it be sufficient to return []string (instead of *[]string) from Get()

@marquiz. This is I think the only part I was wondering about. But you will probably get a much better understanding about what makes sense for Get() and what alternatives make sense for the constructor once you have written some more code exercising this. Then there are two options. Either start with the minimal set your code needs, or with what you guesstimate to be potentially necessary. But this already LGTM as such.

@klihub klihub requested review from chrishenzie and klihub August 21, 2025 15:17
@chrishenzie
Copy link
Contributor

+1 to @klihub 's comment -- would it be possible to add your intended use case as a separate commit in this PR so we can take a look how it's being used? I'd prefer to review that vs. merge now and have to make subsequent changes later

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

agree with your and the other comments.. without the use case, outside the test bucket itself, sort of hard to see whether some of the optional elements are a + or -


assert.Equal(t, tc.expectedValue, result.Get())
})
}
Copy link
Member

Choose a reason for hiding this comment

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

probably want a test that ensures the cloned result.

@klihub klihub marked this pull request as draft September 22, 2025 18:29
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.

4 participants