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

Merged ES_CN and ES_IB parsers #4303

Merged
merged 22 commits into from
Jul 1, 2022

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jun 28, 2022

This PR merges the ES_CN.py and ES_IB.py parsers into a combined ES.py parser.

In this PR:

  • Merged parsers ES_CN.py and ES_IB.py.
  • Reduced code duplication.
  • Logical zone overrides.
  • Improved type safety.
  • Added Ceuta and Melilla
  • Added parsing of past dates (whole days only).
  • Merged and simplified tests.

The reason I wanted to do this is so we don't need to maintain two extremely similar parsers and to open up for adding the Spanish autonomous cities of Ceuta and Melilla in the future.

parsers/ES.py Outdated Show resolved Hide resolved
@VIKTORVAV99 VIKTORVAV99 linked an issue Jun 29, 2022 that may be closed by this pull request
@github-actions github-actions bot added the tests label Jun 30, 2022
@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jun 30, 2022

@madsnedergaard I am holding off on this for a bit until a new version of ree is published (my upstream changes where just merged) but should I archive the parser tests as well or just the parsers?

EDIT:
I archived both, figured it can't hurt and it's easy to remove in the future if we want.

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jun 30, 2022

Hmm I have no idea why the CI check is suddenly failing, it's working for me locally and the changes shouldn't have affected anything.

EDIT:
Looks like the overly specific types conflicted with the more generic types in the code that validates the parsers. I moved back to generic types.

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 1, 2022

@hectorespert Hey could you publish a new version of ree?
Some of the changes in here will use the upstream changes I created PRs for yesterday.
Thanks in advance!

EDIT:
A new release have been published! 🎉

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jul 1, 2022
@VIKTORVAV99 VIKTORVAV99 marked this pull request as ready for review July 1, 2022 08:51
@VIKTORVAV99
Copy link
Member Author

@madsnedergaard This should be ready for review now, the failing CI check appears to be because the test runner is trying to run the archived tests which obviously don't work as the relative imports are broken.

I think this is outside of the scope of this PR and should be fixed in a separate PR but a quick solution would be to delete the archived tests.

(On a side note it looks like every test is run twice? Might want to look into that too if you decide to poke around in the test config.)

@madsnedergaard
Copy link
Member

@madsnedergaard This should be ready for review now, the failing CI check appears to be because the test runner is trying to run the archived tests which obviously don't work as the relative imports are broken.

I think this is outside of the scope of this PR and should be fixed in a separate PR but a quick solution would be to delete the archived tests.

(On a side note it looks like every test is run twice? Might want to look into that too if you decide to poke around in the test config.)

I think deleting the test makes most sense - if we really wanted to we could add a comment in the archived parser with a reference to the commit where it's available... but since we're just merging the two parsers here (and probably don't want to revert to two different ones) I think you could also just delete the parsers entirely :)

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

Yay! 👏
On an unrelated personal note I'm going on 3 months parental leave from today, so if you need to reach someone on the team, @Kongkille is the man 💪

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 1, 2022

Yay! 👏
On an unrelated personal note I'm going on 3 months parental leave from today, so if you need to reach someone on the team, @Kongkille is the man 💪

Have fun with the kid(s)! 🎉
I'll make sure to bother Kongkille with my PRs instead. 😉😁

I'll just do a final check for unused code/variables and then I'll merge this.

@VIKTORVAV99 VIKTORVAV99 merged commit 78f5edb into electricitymaps:master Jul 1, 2022
@VIKTORVAV99 VIKTORVAV99 deleted the merged_ES_parser branch July 1, 2022 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file parser tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsers ES_CN.py and ES_IB.py are almost identical
2 participants