Refactor creating provider in CLI #54
Conversation
rossf7
changed the title
| "github.com/thegreenwebfoundation/grid-intensity-go/pkg/provider" | ||
| ) | ||
|
|
||
| func getClient(providerName string, cacheFile string) (provider.Interface, error) { |
There was a problem hiding this comment.
This is used by both the root and exporter commands to set up a client for the relevant provider.
rossf7
changed the title
| return nil, fmt.Errorf("could not make electricity map provider, %w", err) | ||
| } | ||
| case provider.Ember: | ||
| client, err = provider.NewEmber() |
There was a problem hiding this comment.
We've discussed pulling monthly data as a future feature, now that Ember is now publishing, and this got me thinking.
These New Ember and NewElectricityMap examples are because we have this temporary state of both being in use, right?
In a later release we'd revert to just having one Ember provider, right?
There was a problem hiding this comment.
Hmm I'm not sure on this. I thought we wanted to keep the ElectricityMap provider?
For Ember right now we're using the embedded data but we could extend this provider to fetch the monthly data instead?
Or if we wanted to support both the data file and fetching data I think we'd need to add another arg to grid-intensity to specify a time period. WDYT?
There was a problem hiding this comment.
oh, sorry - I should have been clear - I think we'd still totally keep the electricitymap provider. I was referring to Ember/NewEmber, and should have been more specific.
Yeah, I'm in favour of being able to extend the provider with extra command line args rather than having a separate provider for this. I'll create an issue to discuss that separately though.
There was a problem hiding this comment.
Ah got it thanks for clarifying.
Go doesn't strictly have constructors but this is the equivalent.
https://go.dev/doc/effective_go#composite_literals
I agree better to add optional parameters in the CLI instead of adding another provider.
Towards #44
Last PR for the refactoring.