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

Docs: extend REST API #4492

Merged

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Oct 20, 2020

fixes #4043

Move the documentation on how to extend the AiiDA REST API to the
"Internal Architecture" section.

@ltalirz ltalirz requested a review from giovannipizzi October 20, 2020 15:42
@ltalirz
Copy link
Member Author

ltalirz commented Oct 20, 2020

@giovannipizzi I have edited the original documentation text only very minimally.
I've checked that the example still runs and that the text makes still sense; it could probably be improved further but given the plans to overhaul the REST API, I think it would be a waste of time.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

I agree that it doesn't make sense at this stage to change significantly this.

Beside the comments (mostly formatting), could you also add a new redirect for the old page?

docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
@ltalirz ltalirz force-pushed the issue_4043_restapi_internals branch from 931aa14 to 6c00b6b Compare October 20, 2020 22:08
@ltalirz ltalirz requested a review from giovannipizzi October 20, 2020 22:11
@ltalirz
Copy link
Member Author

ltalirz commented Oct 20, 2020

Beside the comments (mostly formatting), could you also add a new redirect for the old page?

Thanks, redirect added!

@ramirezfranciscof ramirezfranciscof self-assigned this Oct 21, 2020
Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Hi @ltalirz , thanks for the work! I tried to read it thoroughly and made a few comments, could you take a look and tell me what you think?

As an additional general one, could you change the - itemization with *? I know its super nitpicky but its what it says in the guidelines.

docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
docs/source/internals/rest_api.rst Show resolved Hide resolved
docs/source/internals/rest_api.rst Outdated Show resolved Hide resolved
@ltalirz
Copy link
Member Author

ltalirz commented Oct 21, 2020

Hi @ramirezfranciscof thanks for checking the text; I've added a couple of your suggested fixed as well as fixed a broken reference.

Let me know in case you think anything else is necessary - as mentioned above this is a rather specific developer-oriented documentation that will likely become obsolete to some degree once the REST API is touched for the next time.

@ramirezfranciscof
Copy link
Member

Let me know in case you think anything else is necessary - as mentioned above this is a rather specific developer-oriented documentation that will likely become obsolete to some degree once the REST API is touched for the next time.

Ah, I see. Sorry if these were too much corrections then: please rebase the work and I'll merge it.

Also, if this is the case, could I ask you to open an issue mentioning that the section is "temporary" and what other sections might be in the same bag? So that we remember in case people make comments to improve these or don't forget anything when the time comes to make the changes.

Move the documentation on how to extend the AiiDA REST API to the
"Internal Architecture" section.

Co-authored-by: Giovanni Pizzi <gio.piz@gmail.com>
Co-authored-by: ramirezfranciscof <ramirezfranciscof@users.noreply.github.com>
@ltalirz ltalirz force-pushed the issue_4043_restapi_internals branch from bee5e5a to 18d5882 Compare October 22, 2020 08:36
@ltalirz
Copy link
Member Author

ltalirz commented Oct 22, 2020

please rebase the work and I'll merge it.

Done!

Also, if this is the case, could I ask you to open an issue mentioning that the section is "temporary" and what other sections might be in the same bag? So that we remember in case people make comments to improve these or don't forget anything when the time comes to make the changes.

I think it's fine - we will remember when changes are made to the REST API.

This piece of documentation has a very limited target group (developers who want to extend the REST API) - if anyone would like to invest time to improve it, they are welcome, but I think it is not a very important part of the docs (e.g. I started editing the REST API without ever reading it).

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Ready to go!

@ramirezfranciscof ramirezfranciscof merged commit 228716f into aiidateam:develop Oct 22, 2020
@ltalirz ltalirz deleted the issue_4043_restapi_internals branch December 14, 2020 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Internals: REST API
3 participants