export internal/config.Modifier

[jaypipes]

Exports internal/config.Modifier in alias.go so that dependency injection and mocking tools can do a slightly better type-check than any.

Issue #456

[ffromani]

exporting the Modifer LGTM but I concur we should understand the usecase and the pain of the current API better before to merge this PR.

[pastequo]

I jumped in the discussion since I opened the issue.

IMHO, in general, I would avoid exposing a function and put its parameter type in internal/. It's true that helpers are exposed to create the parameter, but anyone using the go package won't be able to create function on top of yours

With that being said, it's true that the function is practically using any, not the internal type. But if any could be avoid, that would be nice

Just my 0.02$

[jaypipes]

I jumped in the discussion since I opened the issue.

IMHO, in general, I would avoid exposing a function and put its parameter type in internal/. It's true that helpers are exposed to create the parameter, but anyone using the go package won't be able to create function on top of yours

With that being said, it's true that the function is practically using any, not the internal type. But if any could be avoid, that would be nice

Just my 0.02$

@pastequo unfortunately, we can't avoid the use of any in the primary module New() functions because that was the only way we were able to make a backwards-compatible function signature since the original ones used a concrete type (the old *pkg/option.Option struct) instead of an interface. :(

Adding internal/config.Modifier as an exported type, however, should allow you to update your mocking to use ghw.Modifer instead of the mock.AnythingOfType(string) method, no?

[pastequo]

I could argue that you are not strictly backward compatible anyway (at least between 0.16.0 & 0.23.0, I haven't looked into the details) because

• 0.16.0 uses *option.Option : https://github.com/jaypipes/ghw/blob/v0.16.0/pkg/product/product.go#L71
• 0.23.0 reads option.Option (no pointer): https://github.com/jaypipes/ghw/blob/v0.23.0/internal/config/context.go#L226
  So my parameters would have been silently ignored

Also another argument "against" the backward compatibility is that it might have worked for 1 option (like New(option.WithXXX)) but it wouldn't have worked for 2 and more
New(option.WithXXX, option.WithYYY) would have raised a compilation error as []Options is not usable as a []any

You are still on major 0, so you can take advantage of that. I think I would have prefer either several New or multiple parameters in New and keep the typing. But that is my personal opinion

Regarding the mock thing, I think it's a limitation of gomock, gwh.Modifer is a function, gomock can't match on them

[jaypipes]

I could argue that you are not strictly backward compatible anyway (at least between 0.16.0 & 0.23.0, I haven't looked into the details) because

• 0.16.0 uses *option.Option : https://github.com/jaypipes/ghw/blob/v0.16.0/pkg/product/product.go#L71
• 0.23.0 reads option.Option (no pointer): https://github.com/jaypipes/ghw/blob/v0.23.0/internal/config/context.go#L226
  So my parameters would have been silently ignored

Sorry, @pastequo. This commit was the one that broke backwards compatibility. :(

option.Option is now a typedef for a function. but used to be a struct. The commit referenced above unfortunately changed the Option struct to be called Options (plural instead of singular) and the Option function typedef accepts a pointer to an Options struct which it mutates. We dropped the ball on this. Really sorry about this!