-
Notifications
You must be signed in to change notification settings - Fork 960
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
Remove JSON configuration files (zones, exchanges, co2 params) #4655
Conversation
…com/electricitymaps/electricitymaps-contrib into ps/remove_references_to_json_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a simple code review and left a few suggestions comments.
Hopefully I'll have some time to test it as well the next few days.
Oh and before I forget myself, we probably need to update the wiki in a few places once this is merged. |
Co-authored-by: Viktor Andersson <[email protected]>
Co-authored-by: Viktor Andersson <[email protected]>
Co-authored-by: Viktor Andersson <[email protected]>
Co-authored-by: Viktor Andersson <[email protected]>
Co-authored-by: Viktor Andersson <[email protected]>
Yes that's on our radar :) |
upsie Co-authored-by: Tony <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from a front end perspective, I've tested the config generation and a manual test of the app.
Not sure if you'd want to do it as part of this PR but we could move all the generated config files to the new config directory. Move world-aggregated.json and excluded-aggregated-exchanges.json (world.json can be deleted too) so we have all config files in one directory.
@@ -1,5 +1,7 @@ | |||
# Note: web/src/helpers.js contains some of the same information. | |||
|
|||
EXCHANGE_FILENAME_ZONE_SEPARATOR = "→" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could just be "-" style dash, exchanges can go both ways so a directional arrow doesn't really fit. Also having a character that doesn't exist on a keyboard is a bit annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be a dash as zone keys already have dashes in them. We could do underscore though and it was discussed before.
The change is minimal so I can include it. I agree about the change.
Aren't the config pieces you're mentioning only used in web?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you on the separator.
Yes they are only used in web, I suggest to follow the same pattern as the now generated exchanges.json and zones.json that live in web/src/config (but are .gitignored). I'd be happy to do this myself in a new PR after this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let you update the web config pieces you were mentioning in a subsequent PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underscore works well 👍
Issue
Follow up on #4435. This removes definitely the now deprecated global .json config.
Description
Preview