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

Issue 2 retire rolling parameter in api decorator #41

Merged
merged 38 commits into from Mar 25, 2021

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Feb 23, 2021

In this PR we:

  • introduce the optional "prior" parameter for POST endpoints (we already had it for GET endpoints)
  • deprecate the use of ISO 8601 repeating time intervals to denote rolling horizons
  • switch to denoting rolling horizons with regular ISO 8601 durations (a breaking API change)

I plan to update all of the API endpoints for POSTing data (meter data, prognoses, prices and weather data), but first I want to get the hardest one right: postPriceData, which forces me to take into account knowledge horizons properly. You probably have some good API coding tips for me at this point already, concerning the use of marshmallow perhaps, before I move on to the other endpoints.

@Flix6x Flix6x added the API label Feb 23, 2021
@Flix6x Flix6x requested a review from nhoening Feb 23, 2021
@Flix6x Flix6x self-assigned this Feb 23, 2021
documentation/api/change_log.rst Show resolved Hide resolved
flexmeasures/api/common/utils/validators.py Show resolved Hide resolved
flexmeasures/api/common/utils/validators.py Outdated Show resolved Hide resolved
flexmeasures/api/common/utils/validators.py Outdated Show resolved Hide resolved
flexmeasures/api/common/utils/validators.py Show resolved Hide resolved
flexmeasures/api/common/utils/validators.py Outdated Show resolved Hide resolved
flexmeasures/api/common/utils/validators.py Outdated Show resolved Hide resolved
flexmeasures/api/v2_0/tests/utils.py Show resolved Hide resolved
flexmeasures/api/v2_0/tests/utils.py Show resolved Hide resolved
nhoening
Copy link
Contributor

@nhoening nhoening commented on 204bf90 Feb 27, 2021

Choose a reason for hiding this comment

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

Neat, I was just looking at that, thinking that should be done better !!

@Flix6x Flix6x marked this pull request as ready for review Mar 19, 2021
@Flix6x Flix6x requested a review from nhoening Mar 19, 2021
@Flix6x Flix6x added Data documentation labels Mar 20, 2021
Copy link
Contributor

@nhoening nhoening left a comment

Looking pretty good - almost there :)

  • it would be good to add a line to the overall changelog, but I'm not sure it exists already in this branch. Maybe you'd merge main into this one anyways when you deal with merge conflicts ...
  • The chosen name for the new field (prior) makes most sense when used for GETting data (as a query parameter). For POSTing, a bit less so (for me). How about using the name known_at?

documentation/api/change_log.rst Outdated Show resolved Hide resolved
documentation/api/introduction.rst Outdated Show resolved Hide resolved
flexmeasures/api/common/utils/api_utils.py Outdated Show resolved Hide resolved
documentation/api/introduction.rst Outdated Show resolved Hide resolved
flexmeasures/api/common/utils/api_utils.py Show resolved Hide resolved
@Flix6x
Copy link
Contributor Author

@Flix6x Flix6x commented Mar 23, 2021

I'd like to keep the name for the prior field for now. Terminology is a big issue in itself. We can revisit the name later if needed, but I would like to keep terminology consistent across FlexMeasures (also with respect to other fields) and that deserves a discussion of its own.

@nhoening
Copy link
Contributor

@nhoening nhoening commented Mar 23, 2021

Keeping terminology consistent would require discussing a term well before merging the PR that introduces it to main, no?

@Flix6x
Copy link
Contributor Author

@Flix6x Flix6x commented Mar 24, 2021

The "prior" field is already used on main denoting the equivalent concept for GET requests, so for me, keeping terminology consistent is naming this field for POST requests the same. I cannot follow up on your suggestion for renaming the prior field for POST requests without a discussion of renaming the equivalent field for GET requests, too, as well as the closely related "horizon" field used for both GET and POST requests.

@nhoening
Copy link
Contributor

@nhoening nhoening commented Mar 24, 2021

I overlooked that it is on main already. Go on and merge. Let's briefly discuss my naming idea in person / on the phone.

@Flix6x Flix6x merged commit 48367b8 into main Mar 25, 2021
2 checks passed
@Flix6x Flix6x deleted the issue-2-Retire_rolling_parameter_in_API_decorator branch Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Data documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants