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

Update reporting after renaming v33_emi to vm_emiCdrTeDetail #503

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

tabeado
Copy link
Contributor

@tabeado tabeado commented Jan 10, 2024

Update reporting after renaming v33_emi to vm_emiCdrTeDetail in remindmodel/remind#1517

@tabeado tabeado requested review from amerfort and katarkow January 10, 2024 10:00
@amerfort
Copy link
Contributor

amerfort commented Jan 10, 2024

The reporting is supposed to have a certain backwards-compatibility, right? So we would probably have to have a check if v33_emi still exists (because the respective gdx is a little older) and do the calculation the old way in this case. @katarkow would you agree?

@tabeado
Copy link
Contributor Author

tabeado commented Jan 10, 2024

@amerfort My idea was:
In line 176 R is looking for either v33_emi or vm_emiCdrTeDetail (I copied this from other parts of the code). If we have the newest gdx version after the renaming, it only has vm_emiCdrTeDetail, if it is before that it finds v33_emi. If it is before the refactoring, Kasia's following code covers it. All these cases now feed into the R element "vm_emiCdrTeDetail", so we don't have to take care of it in other parts of the code.
Does this miss anything?

@amerfort
Copy link
Contributor

Oh I totally missed that in the PR, I'm so sorry! Thanks for taking care of this and thanks for explaining!

Copy link
Contributor

@amerfort amerfort left a comment

Choose a reason for hiding this comment

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

Thank you very much for taking care of this! Looks good to me.

@tabeado tabeado merged commit f33dd29 into pik-piam:master Jan 12, 2024
4 checks passed
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.

2 participants