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

Added JsonForecast datasource #312

Closed

Conversation

in4margaret
Copy link
Contributor

@in4margaret in4margaret commented Mar 5, 2023

Pull Request

Issue Number: (Link to Github Issue or Azure Dev Ops Task/Story)

Summary

Added JsonForecast datasource

Changes

  • JsonDataSource.cs implements IForecastDataSource.
  • AzureRegionTestDataGenerator.cs generates data compatible with IForecastDataSource implementation for JsonDataSource
  • Json Data Source Mocks generates data needed for Forecast Integration tests.
  • Forecast Endpoints are enabled for Json
  • JSON unit tests pass
  • Integration tests with Json pass for Emissions & Forecast endpoints
  • Documentation is updated
  • Remove Newtonsoft package from CarbonAware.Tools.AzureRegionTestDataGenerator and use System.Text.Json instead
  • Changed ServiceCollectionExtensions from JSON datasource to match signature of other datasources (return IServiceCollection instead of void).

Checklist

  • Local Tests Passing?
  • CICD and Pipeline Tests Passing?
  • Added any new Tests?
  • Documentation Updates Made?
  • Are there any API Changes? If yes, please describe below.
  • This is not a breaking change. If it is, please describe it below.

Are there API Changes?

If yes, what are the expected API Changes? Please link to an API-Comparison
workflow with the API Diff.

Is this a breaking change?

If yes, what workflow does this break?

Anything else?

Other comments, collaborators, etc.

Please follow
GitHub's suggested syntax
to link Pull Requests to Issues via keywords

This PR Closes #<issue_number>

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2023

Codecov Report

Merging #312 (4d4ede3) into dev (43de9b0) will increase coverage by 0.13%.
The diff coverage is 84.41%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #312      +/-   ##
==========================================
+ Coverage   81.73%   81.86%   +0.13%     
==========================================
  Files          69       69              
  Lines        2392     2459      +67     
  Branches      244      252       +8     
==========================================
+ Hits         1955     2013      +58     
- Misses        355      360       +5     
- Partials       82       86       +4     
Impacted Files Coverage Δ
...s.ElectricityMaps/src/ElectricityMapsDataSource.cs 64.70% <ø> (+0.62%) ⬆️
...n/src/Configuration/JsonDataSourceConfiguration.cs 87.50% <ø> (ø)
...CarbonAware.DataSources.Json/src/JsonDataSource.cs 71.42% <64.51%> (-8.58%) ⬇️
...ware.DataSources.Json/mock/JsonDataSourceMocker.cs 95.08% <97.05%> (+5.08%) ⬆️
...n/src/Configuration/ServiceCollectionExtensions.cs 100.00% <100.00%> (ø)
...ation/Configuration/ServiceCollectionExtensions.cs 77.77% <100.00%> (+7.32%) ⬆️
src/CarbonAware/src/Model/EmissionsJsonFile.cs 100.00% <100.00%> (ø)

Copy link
Contributor

@Kpopovs1 Kpopovs1 left a comment

Choose a reason for hiding this comment

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

Moving from Newtonsoft to System.Text.Json was a good catch! I just added few comments.

Copy link
Contributor

@vaughanknight vaughanknight left a comment

Choose a reason for hiding this comment

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

These changes look great. Awesome gap to fill to so thank you. Very much appreciated.

@vaughanknight
Copy link
Contributor

From the errors it looks like the test data json file might have issues, I need to look into it. I don't think it's a code issue at this time, but likely a test data file issue.

@Willmish
Copy link
Collaborator

@vaughanknight could you confirm whether we are holding off with merging this until after the release?

@Willmish
Copy link
Collaborator

bringing this back up, to confirm whether fixing this is being held off until post v1.1 @vaughanknight

@Willmish
Copy link
Collaborator

From #405 : currently available to pick up for review

@Willmish
Copy link
Collaborator

#417 To decide whether to close this off on next meeting - has been broken for a while and no one volunteered to fix/pick this up.

@danuw
Copy link
Collaborator

danuw commented Dec 5, 2023

This has been breaking for too long and is not a critical feature. It was a great contribution we will revisit using the latest code.
We will be abandoning this version for now.

@danuw danuw closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants