-
Notifications
You must be signed in to change notification settings - Fork 46
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
TIME_PERIOD instead of time #285
Comments
Eurostat changed its API indeed. We will need to investigate if this comes from there directly; if yes then the main problem is not in the software and it will require some discussion on should we support conversions between old and new version (since those changes may have a reason..); or another option is that we forgot to add deprecation message (which should maintain the previous functionality). Now is the holiday break but we aim to come back to this asap. Meanwhile, further info and suggestions are welcome. |
I think that this will be an issue in every package that uses eurostat, it needed a fix in iotables and regions, too. The eurostat package does not use standard SDMX/Eurostat variable names, so I do not see any reason not to keep the time variable name instad of TIME_PERIOD. The logical solution would be to keep backward compatibility with the package, and offer to use standard variable names (but not only with TIME _PERIOD) |
Not that I am whipping and hurrying you with the issue, but could we expect a decision in this regard relatively soon? I need to know whether I should start changing codes in files I use for work. |
Yes we can expect the decision soon. This week people are arriving from vacations and it takes some days to catch up. We will talk with @pitkant |
Thank you for opening this issue @rideofyourlife . Renaming time column to TIME_PERIOD was the single most breaking change of the 4.0.0 version. Such big changes can be expected when a major release is made, and jump from 3.8.3 to 4.0.0 is a major release according to Semantic Versioning practice. However, that does not mean that the update couldn't be handled better, with some features helping with backwards compatibility. Therefore I apologise for any inconvenience this change has caused. While it does not solve your problem, here is my rationale of why the change was made:
Of course we can release a new version of the package that could remedy this issue. I would personally think having an additional argument in the |
If this helps our users to not break their existing workflows that would sound worth doing. Over the longer term things must be deprecated as well but a transition period can be very useful. |
I do not share this opinion. "time" vs "TIME_PERIOD":
|
Again, apologies for the trouble caused. I do still feel that in the long term following and mirroring the API changes is the right thing to do. If you disagree with that maybe we can open a Discussion instead of an issue for discussing how to handle similar issues in the future. In the meantime, I think we can fix this specific with a temporary argument to |
Alright, while I was debugging something else I noticed that what I said earlier in this discussion is at conflict with how the package actually currently works: SDMX API:
API Statistics:
Getting years as chr from The reason for this is that The conclusion being that I would make the new |
Already posted on #288 but I'll put this here as well: @rideofyourlife I have uploaded some WIP code in v4.1 branch. It enables users to make queries the same way as before but adds an additional parameter legacy.data.output to get_* functions that transforms dimensions names such as TIME_PERIOD and OBS_VALUE to time and values that were used before and removes extra columns such as freq, DATAFLOW and LAST UPDATE altogether. Related to what I wrote earlier here:
I would maybe back out on this as it is much simpler to use Eurostat conventions internally and then change the data output just before the object is returned to the user, after all data labelling etc. tasks are done. If you could test this and give some feedback on what you think it would be great! |
It seems that in one of the latest updates "time" parameter from get_eurostat() has been changed into "TIME_PERIOD". Obviously this is troublesome for those who have already written thousands of lines of code. The change seems to add no value for the package whatsoever. Is this because of changes that Eurostat implemented in its' API?
The text was updated successfully, but these errors were encountered: