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 58 api documentation lists each endpoint twice #59

Merged
merged 4 commits into from Mar 11, 2021

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Mar 9, 2021

closes #58

@nhoening nhoening requested a review from Flix6x Mar 10, 2021
Flix6x
Flix6x approved these changes Mar 10, 2021
@@ -271,7 +271,7 @@ def post_weather_data():
def get_prognosis():
"""API endpoint to get prognosis.
.. :quickref: User; Download prognosis from the platform
.. :quickref: Control; Download prognosis from the platform
Copy link
Contributor

@Flix6x Flix6x Mar 10, 2021

Choose a reason for hiding this comment

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

Maybe I don't understand the logic behind your chosen categories, but I think this should be "Data" instead of "Control". Similarly for POSTing prognoses.

Copy link
Contributor Author

@nhoening nhoening Mar 11, 2021

Choose a reason for hiding this comment

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

Yes, I'm on the fence with this one. Not sure if we need those labels for a long time and that it matters much. I'll go with your suggestion.

@@ -62,7 +62,7 @@
def get_connection():
"""API endpoint to get the user's connections as entity addresses ordered from newest to oldest.
.. :quickref: User; Retrieve entity addresses of connections
.. :quickref: Data; Retrieve entity addresses of connections
Copy link
Contributor

@Flix6x Flix6x Mar 10, 2021

Choose a reason for hiding this comment

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

I'd reserve the "Data" quickref for time series data only. "Assets" fits better here.

Copy link
Contributor Author

@nhoening nhoening Mar 11, 2021

Choose a reason for hiding this comment

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

I agree.

@nhoening nhoening merged commit 388a8a7 into main Mar 11, 2021
2 checks passed
@nhoening nhoening deleted the issue-58-API_documentation_lists_each_endpoint_twice branch Mar 11, 2021
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