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

Improved docs of Plone REST API JavaScript Client #5576

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Hrittik20
Copy link
Contributor

Issue: #5491

Copy link

netlify bot commented Dec 25, 2023

Deploy Preview for plone-components ready!

Name Link
🔨 Latest commit 80bf229
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/65a6badc9cc346000859258a
😎 Deploy Preview https://deploy-preview-5576--plone-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 25, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 80bf229
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65a6badcff454400082ce701
😎 Deploy Preview https://deploy-preview-5576--volto.netlify.app/recipes/developing-a-project.html
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mister-roboto
Copy link

@Hrittik20 you need to sign the Plone Contributor Agreement to merge this pull request.

Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement

@stevepiercy
Copy link
Collaborator

@Hrittik20 did you sign and return the Plone Contributor Agreement? When you are notified that you have been added to the Contributors Team, please add a comment to let us know.

@Hrittik20
Copy link
Contributor Author

@stevepiercy Yes, I have recieved the agreement in my email.

@stevepiercy
Copy link
Collaborator

@Hrittik20 then there is a mismatch between your email registered in GitHub and that which you submitted in your Plone Contributor Agreement. They must match, else @mister-roboto will forever nag you to sign the Plone Contributor Agreement. Please make sure they match, or add an email to your GitHub account that matches what you submitted.

@Hrittik20
Copy link
Contributor Author

@stevepiercy I checked but my email and my github username is correct. So I dont know why its not working.

@stevepiercy
Copy link
Collaborator

@Hrittik20 please contact agreements@plone.org and ask what happened. Perhaps they made a typo?

@@ -2,9 +2,10 @@

A mechanism to redirect old URLs to new ones.

When an object is moved (renamed or cut/pasted into a different location), the redirection storage will remember the old path. It is smart enough to deal with transitive references (if we have a -> b and then add b -> c, it is replaced by a reference a -> c) and circular references (attempting to add a -> a does nothing).
When an object is moved (renamed or cut/pasted into a different location), the redirection storage will remember the old path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
When an object is moved (renamed or cut/pasted into a different location), the redirection storage will remember the old path.
When an object is moved (renamed or cut-and-pasted into a different location), the redirection storage will remember the old path.

@@ -2,9 +2,10 @@

A mechanism to redirect old URLs to new ones.

When an object is moved (renamed or cut/pasted into a different location), the redirection storage will remember the old path. It is smart enough to deal with transitive references (if we have a -> b and then add b -> c, it is replaced by a reference a -> c) and circular references (attempting to add a -> a does nothing).
When an object is moved (renamed or cut/pasted into a different location), the redirection storage will remember the old path.
Handles transitive references (for example a -> b, b -> c becomes a -> c) and ignores circular ones (for example attempting a -> a has no effect).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Handles transitive references (for example a -> b, b -> c becomes a -> c) and ignores circular ones (for example attempting a -> a has no effect).
It handles transitive references intelligently (for example a -> b, b -> c becomes a -> c), ignoring circular ones (for example attempting a -> a has no effect).


## Add aliases for many pages

### Mutation function
```{js:function} createAliasesMutation(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The next three went sideways. See example: https://deploy-preview-5576--volto.netlify.app/client/actions/aliases.html#add-aliases-for-many-pages

I'm not sure how to document the structure of an object as an argument. If you don't have any better idea, I'd try this (untested):

:arg array data: It can have the following fields:
  - items: An array of objects with the following fields:
    - path: string
    - redirect_to: string
    - [datetime]: string

You might need to indent the list items to the right amount. I don't know if Markdown for list items will even work in the value for arguments.

The ultimate fallback would be to convert the object into narrative text, which is not great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, lets try it and see if it's better

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also do make docs-livehtml to preview docs in real-time. It's a lot faster than waiting for Netlify. It still doesn't look quite right. https://deploy-preview-5576--volto.netlify.app/client/actions/aliases.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevepiercy I tried doing make docs-livehtml but unfortunately I am getting this error :
/usr/bin/sh: line 2: bin/python: No such file or directory make: *** [Makefile:99: bin/python] Error 127

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which way did you use to set up your environment for Volto docs? There are two ways, depending on the scope.

It looks like you do not have a Python virtual environment installed, according to this:

/usr/bin/sh: line 2: bin/python: No such file or directory make: *** [Makefile:99: bin/python] Error 127

Line 99 in Makefile has this:

	python3 -m venv . || virtualenv --clear --python=python3 .

which means "Create a Python 3 virtual environment in the current directory, if possible, else use virtualenv to wipe and recreate a Python 3 virtual environment." It fails because you either do not have Python installed on your system or, if you do have it installed, Python is not in your system $PATH.

If you do not have Python installed, see the advice to install Python. Else you will need to ensure that your $PATH is configured to point to a Python on your system. Please let me know.

- redirect_to: string
- [datetime]: string
:hook: `useCreateAliases`
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we have an easy and useful way to show data structures in narrative text through docstrings. Here's my latest attempt, but it is still unsatisfactory:

## Add aliases for many pages

```{js:function} createAliasesMutation(data)

Use the `createAliasesMutation` function to get the mutation for adding aliases for many pages.

:arg array data: It can have the following fields:

                 `items`
                 : An array of objects with the following fields:

                     -   `path`: string
                     -   `redirect_to`: string
                     -   \[`datetime`\]: string

:hook: `useCreateAliases`
Screenshot 2024-01-07 at 5 21 35 AM

That Markdown is horrible and difficult to write and maintain, and the result still does not look correct.

Perhaps we should instead adopt the practice of what we do in the Plone REST API, where we show the data structure to send as a POST request with an expected response?

https://6.docs.plone.org/plone.restapi/docs/source/endpoints/aliases.html#adding-url-aliases-in-bulk

@Hrittik20 what do you think?

Before going further, I think I want to chat with @sneridagh to discuss how we can both improve this package's documentation and better integrate with the Plone REST API docs. That conversation might yield a better direction. I ask for your patience until we can work through this. It might take until our next Volto Team meeting, which you are welcome to join. See https://discord.com/channels/786421998426521600/787308038050545666/1191701430642430054 for details.

I apologize that I did not realize that this would become more complicated, but I want to thank you for helping me come to this realization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevepiercy Absolutely, I agree with your suggestion to improve the documentation following the Plone REST API approach. Thanks for your assistance throughout this matter. Also, can you share the discord link again (link expired probably)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@stevepiercy
Copy link
Collaborator

@Hrittik20 sorry for the long delay, and thank you for your patience. I chatted with @sneridagh last week, and we came up with a plan.

First, take a look at Endpoints in the Plone REST API docs. Note that these endpoints correspond to the client methods, sort of.

Using Adding URL aliases in bulk, we want to add a new tab to the four existing tabs, "JavaScript", that would show the JavaScript client method createAliasesMutation that the developer would use to call the Plone REST API endpoint. Thus the battle of trying to format and describe the data structure would be replaced with a link to that REST API endpoint.

@sneridagh did I capture that correctly? My notes are vague, and I realized that this plan might need some refinement and further discussion. Please verify.

@Hrittik20
Copy link
Contributor Author

@stevepiercy Can you tell me a bit more about how I can add new tab to the existing tabs using "Adding URL aliases in bulk"?

@stevepiercy
Copy link
Collaborator

@Hrittik20 assuming you cloned the documentation repo, you should also have the plone.restapi repo in a submodule. Endpoint documentation is located at submodules/plone.restapi/docs/source/endpoints.

There are two Sphinx extensions that enable the tabs and examples. From requirements.txt:

sphinxcontrib.httpdomain  # plone.restapi
sphinxcontrib.httpexample  # plone.restapi

More information:

@Hrittik20
Copy link
Contributor Author

@stevepiercy Okay. Should I use "sphinx-tabs" extension to build the new tabs or is there any other better way?

@stevepiercy
Copy link
Collaborator

The extension sphinxcontrib-httpexample which I mentioned provides tabs already. It could be extended for the JavaScript client.

@sneridagh
Copy link
Member

sneridagh commented Jan 30, 2024

@stevepiercy yeah, you explained it right. What we said also is that afterwards we could remove all the endpoints docs here, to point to the main endpoints documentation in plone.restapi, and here leave only the introduction, basics, install.

@Hrittik20
Copy link
Contributor Author

Hrittik20 commented Jan 31, 2024

@stevepiercy Can you guide me in extending sphinxcontrib-httpexample for JavaScript client? Currently, I think it mostly supports curl, wget, python-requests, and httpie. Your insights would be appreciated.

@stevepiercy
Copy link
Collaborator

@Hrittik20 I have zero familiarity with how it works. I am just an end user. In general, would duplicate what is done for one of the other request methods, writing code and tests.

You could either investigate on your own, ask on the Community Forum, or contact the maintainers (https://pypi.org/project/sphinxcontrib-httpexample/) or contributors to it.

@Hrittik20
Copy link
Contributor Author

@stevepiercy To add a JavaScript client tab, I would need to modify sphinxcontrib-httpexample. We can either create a modified fork of sphinxcontrib-httpexample or make a local package. I think a local package would be a better option. What do you think?

Additionally, the JavaScript client tab should display the string 'createAliasesMutation', correct?

@stevepiercy
Copy link
Collaborator

@Hrittik20 you will need to create a fork of that project, just as you always do when contributing to an open source software project. See https://collective.github.io/ for how to contribute to the organization's repos. You could follow Plone's contributing documentation, as Collective's is somewhat lacking.

It would also be a good idea to create a new issue in that repo, referencing this pull request, to explain what we would like to do. There are a couple of old issues from 2017 suggesting the addition of other language's request tools, too.

@Hrittik20
Copy link
Contributor Author

@stevepiercy Is this okay?

Capture

@stevepiercy
Copy link
Collaborator

@Hrittik20 yup, that's a good start.

@Hrittik20
Copy link
Contributor Author

@stevepiercy Should I create a pull request, or are there additional changes I need to make?

@stevepiercy
Copy link
Collaborator

I would expect to see more content in the tab, such as a request body, similar to the http tab's content, but formatted for the JavaScript library. See example https://6.docs.plone.org/plone.restapi/docs/source/endpoints/aliases.html#adding-new-url-aliases-for-a-page

@Hrittik20
Copy link
Contributor Author

@stevepiercy I made the modifications. Is this better?

Capture

@stevepiercy
Copy link
Collaborator

@Hrittik20 yes, it looks much better. It would be good to format the JSON object for readability, similar to what the http request and response do. Example: https://6.docs.plone.org/plone.restapi/docs/source/endpoints/aliases.html#adding-new-url-aliases-for-a-page

@Hrittik20
Copy link
Contributor Author

@stevepiercy It took me a while to figure out the format 😅

Capture

@stevepiercy
Copy link
Collaborator

Yeah, I saw your SO post. 😄

Is it ready for a pull request? It does not have to be perfect, as the maintainers may make suggestions during review.

@Hrittik20
Copy link
Contributor Author

Yes, I will make the pull request for sphinx now

@stevepiercy
Copy link
Collaborator

@Hrittik20 the PR should not go in Sphinx, but in the Sphinx extension https://github.com/collective/sphinxcontrib-httpexample.

@Hrittik20
Copy link
Contributor Author

@stevepiercy The pull request has been merged. You can also check the documentation at https://sphinxcontrib-httpexample.readthedocs.io/en/latest/usage.html for the JavaScript tab usage.

@stevepiercy
Copy link
Collaborator

@Hrittik20 I am super excited! @sneridagh check out the new JavaScript tab that we can now apply to the REST API docs.

https://sphinxcontrib-httpexample.readthedocs.io/en/latest/usage.html

My work this week is crazy busy, but I look forward to the next steps. Thank you! Thank you! Thank you!

@sneridagh
Copy link
Member

@Hrittik20 looks already good! Can't wait to see the final outcome!

Did you think about how to pass the data to the tab? We can discuss the options that we have. For starters, they should not collide with the existing ones, and complement them in a way that is not intrusive and break them.

FTR, in case you don't stumbled upon them:

https://github.com/plone/plone.restapi/blob/main/src/plone/restapi/tests/test_documentation.py#L2411-L2417

in this test is how we generate the sphinx extension data. Then the docs consume it:

https://github.com/plone/plone.restapi/blob/main/docs/source/endpoints/vocabularies.md?plain=1#L90-L92

Maybe we can include another marker like :javascript: then pass a file with the code? Wondering if we could autogenerate it out of the other existing information... I will think about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: No status
Status: In Progress
Status: Pending
Development

Successfully merging this pull request may close these issues.

None yet

4 participants