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

Make Ember a provider for use in exporter #24

Merged
merged 6 commits into from
Jun 23, 2022
Merged

Make Ember a provider for use in exporter #24

merged 6 commits into from
Jun 23, 2022

Conversation

rossf7
Copy link
Contributor

@rossf7 rossf7 commented Jun 22, 2022

@rossf7 rossf7 marked this pull request as ready for review June 22, 2022 13:43
api/provider.go Outdated
@@ -5,5 +5,6 @@ import (
)

type Provider interface {
GetAllRegionsCarbonIntensity(ctx context.Context) (map[string]float64, error)
Copy link
Contributor Author

@rossf7 rossf7 Jun 22, 2022

Choose a reason for hiding this comment

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

This adds a new method to the interface for fetching all counties and their carbon intensity.

To add year as we're discussing in thegreenwebfoundation/grid-intensity-exporter#7 we'll need to change the returned types here.

But I'd prefer to do that as a follow up as these changes are already quite big. Is that OK?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's defer that decision.

@@ -41,6 +41,10 @@ type ApiClient struct {
token string
}

func (a *ApiClient) GetAllRegionsCarbonIntensity(ctx context.Context) (map[string]float64, error) {
return nil, ErrNotSupported
}
Copy link
Contributor Author

@rossf7 rossf7 Jun 22, 2022

Choose a reason for hiding this comment

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

For ElectricityMap this would involve a lot of API calls IIUC

So we just error if someone tries to use this.

ember/data.go Outdated
}

data[countryCode] = GridIntensity{
CountryCode: countryCode,
Copy link
Member

Choose a reason for hiding this comment

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

Hey Ross - I've linked to CSV data here with both sets of country codes. If we had both sets here, we'd be able to support 2 and 3 character lookups.

thegreenwebfoundation/grid-intensity-exporter#7 (comment)

The columns here are comparable, but I'm not sure if we should include the column about emissions type - there are definite differences between marginal and average carbon intensity, but that might be better managed at the provider choice instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being able to do lookups with either format would be really useful.

I'll add the 2 char codes to the current file from the CSV you provided. It would also be great if Ember can add that upstream.

@mrchrisadams
Copy link
Member

Hey Ross, I'm really happy with this.

I agree with the decisions to defer the date / time discussion to another PR

@mrchrisadams
Copy link
Member

Feel free to merge whenever you're comfortable doing so.

@rossf7
Copy link
Contributor Author

rossf7 commented Jun 23, 2022

The data file now has both the 2 and 3 char country codes. I wrote a little go program to map them from the CSV data you gave me. I'll add it as a follow up PR.

I've tested with the exporter and looks fine so going to go with this.

@rossf7 rossf7 merged commit 17dc0dd into main Jun 23, 2022
@rossf7 rossf7 deleted the ember-api branch June 23, 2022 10:56
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

2 participants