search

jaypipes / ghw

Go HardWare discovery/inspection library
Apache License 2.0
1.62k stars 174 forks source link

allow passing in existing Context as an Option #280

Closed jaypipes closed 2 years ago

jaypipes commented 2 years ago

This patch simplifies some internal-only code that was added to ghw to prevent re-creating Context objects inefficiently during snapshot creation. We add a context.WithContext function that returns an option.Option containing a pre-existing Context struct pointer, embedded in the Option as a raw interface{} to get around package recursive import issues.

Now, when packages like pkg/gpu or pkg/pci need to load the pkg/topology package, they call topology.New(context.WithContext(ctx)), passing in the already-existing Context object. This preserves the original New function signature without adding a new NewWithContext function signature to all packages.

Alternative to #278

Signed-off-by: Jay Pipes jaypipes@gmail.com

jaypipes commented 2 years ago

Comparing this PR with my #278 is a quite hard call. The things I don't like in this PR are the awkward usage of interface{} into option.Option, and the type check + panic in context.Context. It is pretty much to avoid them that I introduced the clumsy NewWithContext API. This PR however is much more self contained, and treats a specific, uncommon flow as such instead of sprinking changes all over the codebase to support that flow.

Yeah, I totally hear you. It's a bit of choosing between two not-great options, I agree.

From the whole project perspective, I think this PR (bar the comment inside about the interaction between topology and pci) is a tad better because it is indeed more self contained and in general follows more the existing direction and feeling of the ghw API.

Furthermore, we're not adding new APIs here (opposite to what I was doing). This is more and more relevant as 1.0.0 approaches. Should we grow not too happy about the approach implemented here, we can add APIs later; OTOH removing them is always harder.

Yes, agreed.

Last but not least, I think that we should -in due time- just take the chance to revisit the way the option and context package interact and maybe redesign them. This is easier done going with this approach (minimal change) than with mine (more API surface). Yet another reason to prefer this change over #278.

We should combine the context and option packages, yes. And I plan to do that before 1.0.

ffromani commented 2 years ago

From the whole project perspective, I think this PR (bar the comment inside about the interaction between topology and pci) is a tad better because it is indeed more self contained and in general follows more the existing direction and feeling of the ghw API. Furthermore, we're not adding new APIs here (opposite to what I was doing). This is more and more relevant as 1.0.0 approaches. Should we grow not too happy about the approach implemented here, we can add APIs later; OTOH removing them is always harder.

Yes, agreed.

Indeed we agree, so we have a way forward! Let's pick this PR over https://github.com/jaypipes/ghw/pull/278.The only missing item is how we want to handle https://github.com/jaypipes/ghw/commit/691659d5a0f68705b4ca7f3b4e285d22ad7a6cf2#diff-a6b7c579eae86e07718647003b278fce2940fccd215fa7db50f05f697691847fR158 - I'm fine with any approach from changing this PR to a followup one. Once this is settled I'll close my #278

Last but not least, I think that we should -in due time- just take the chance to revisit the way the option and context package interact and maybe redesign them. This is easier done going with this approach (minimal change) than with mine (more API surface). Yet another reason to prefer this change over #278.

We should combine the context and option packages, yes. And I plan to do that before 1.0.

Awesome news!