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

Switch documentation to JSDoc and Sphinx #997

Merged
merged 37 commits into from
Oct 6, 2017
Merged

Switch documentation to JSDoc and Sphinx #997

merged 37 commits into from
Oct 6, 2017

Conversation

raucao
Copy link
Member

@raucao raucao commented Oct 2, 2017

I've been working on considerably improving the documentation situation in recent weeks. It's now at a point, where I think other people can and should have a look at it, and any feedback or contributions would be most appreciated.

You can find all relevant information about how the new docs are written and generated in the new docs themselves: https://github.com/remotestorage/remotestorage.js/blob/docs/sphinx/doc/contributing/docs.rst

Warning

Unfortunately, one part of the puzzle is still somewhat missing: Read the Docs haven't updated their build images on all production workers yet, so the building and publishing to what is supposed to be the new public documentation website at https://remotestoragejs.readthedocs.io/ fails most of the time. (See readthedocs/readthedocs.org#3069 for details.)

refs #972, #801, #785, #748, #732, #707
closes #864

@galfert
Copy link
Member

galfert commented Oct 4, 2017

I don't seem to have the sphinx-autobuild command that is mentioned in doc/contributing/docs.rst.

@galfert
Copy link
Member

galfert commented Oct 4, 2017

sphinx-autobuild seems to be a separate requirement. I'll add it to doc/requirements.txt.

@galfert
Copy link
Member

galfert commented Oct 4, 2017

I also had to add pip install sphinx_rtd_theme to the requirements.txt, otherwise I got this error when running sphinx-autobuild:

Theme error:
sphinx_rtd_theme is no longer a hard dependency since version 1.4.0. Please install it manually.(pip install sphinx_rtd_theme)

@raucao
Copy link
Member Author

raucao commented Oct 4, 2017

@galfert Great, thanks!

I also just moved all untouched legacy docs to the same folder/namespace, so that they're all in one place.

@galfert
Copy link
Member

galfert commented Oct 4, 2017

Building and serving the docs works for me now. I just get this warning:

WARNING: html_static_path entry u'remotestorage.js/doc/_static' does not exist

And when browsing the documentation, I can see this in the logs:

[W 171004 16:22:56 web:2063] 404 GET /_static/fonts/fontawesome-webfont.woff2?v=4.6.3 (127.0.0.1) 4.01ms

Other than that it looks good. Great work @skddc 👍

@raucao
Copy link
Member Author

raucao commented Oct 4, 2017

You can just create that _static directory in doc/ to eliminate the warning (will be ignored). Looks like an issue with the RTD theme extension.

@raucao
Copy link
Member Author

raucao commented Oct 4, 2017

@galfert I think it would be a good idea if you ported the rest of the public JS API (caching, access, not sure what else) to the new docs, in order to get accustomed to the new setup. Then we have at least two people who can help other contributors with it from here on.

@raucao
Copy link
Member Author

raucao commented Oct 4, 2017

By the way, we've been lucky with the RTD builds today. Some still failed, but the last 3 passed, so whoever's interested can also preview the current state of the new docs over there:

http://remotestoragejs.readthedocs.io/

@galfert
Copy link
Member

galfert commented Oct 4, 2017

@galfert I think it would be a good idea if you ported the rest of the public JS API (caching, access, not sure what else) to the new docs, in order to get accustomed to the new setup. Then we have at least two people who can help other contributors with it from here on.

Ok, I will give it a try.


};
/**
* TODO: do we still need this, now that we always instantiate the prototype?
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get the question, but I think we still need this for enabling/disabling the logging during runtime (i.e. after initial creation of the remoteStorage instance).

Copy link
Member Author

@raucao raucao Oct 4, 2017

Choose a reason for hiding this comment

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

Yes, but my question was why would you do that? Or, in other words, did you ever do that yourself? And if so, did you not just do it, because you couldn't instantiate the instance?

It would be lost after a reload, no? So you'd have added a line to your code enabling the logs, instead of using this from a console. And then you might as well add it to the configuration object upon initialization was the point.

Copy link
Member

Choose a reason for hiding this comment

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

I can't really anticipate all use-cases others might have. I myself used it a few times for debugging, enabling the log for a certain action and then disabling it again so it's not so noisy.

Copy link
Member Author

@raucao raucao Oct 5, 2017

Choose a reason for hiding this comment

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

It's not really about anticipating, but more like giving away clothes you haven't used in the last 12 months. Enabling the logs for a certain action and then disabling before reload seems like a rather rare use case to me.

However, adding this function to app code, instead of the init config, and then losing lots of logs without realizing it (which happened to me every time I used it myself) seems like an actual problem we should try to not have people run into.

@raucao
Copy link
Member Author

raucao commented Oct 5, 2017

@galfert I'm pretty sure everything but claim() is private for Access. If we actually want people to manually manage the prototype's access config, we might at least want to add notes, or a second header, in order to ensure nobody thinks that these are all necessary or reasonable to integrate.


.. code:: javascript

remoteStorage.caching.set('/bookmarks/archive')
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is missing a strategy, because usually you'd use enable() if you don't have any explicit setting in mind.

Copy link
Member Author

@raucao raucao left a comment

Choose a reason for hiding this comment

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

Most functions in Caching are missing examples. Also, I'm not sure if we should advertise all of them as being useful or even public. Anything public should have an example and enough explanation of how/why to use it, including an example imo.

remoteStorage.caching.set('/bookmarks/archive')

.. autofunction:: Caching#enable
:short-name:
Copy link
Member Author

Choose a reason for hiding this comment

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

Example would be nice.

@raucao
Copy link
Member Author

raucao commented Oct 5, 2017

@galfert Are we actually certain that the caching strategy options are up to date?

@@ -0,0 +1,46 @@
Caching
=======
Copy link
Member Author

Choose a reason for hiding this comment

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

I think an introductory note that is initialized as remoteStorage.caching, but only if one doesn't configure caching: false, would be useful.

@galfert
Copy link
Member

galfert commented Oct 5, 2017

@galfert Are we actually certain that the caching strategy options are up to date?

I couldn't find anything that would indicate otherwise.

@raucao
Copy link
Member Author

raucao commented Oct 6, 2017

Note: we decided in the IRC meeting yesterday to merge this asap. I don't see why this couldn't live in master tbh, so one can branch off from that again for all the remaining docs issues. Removing WIP. Still needs rebase tho.

@raucao raucao changed the title [WIP] Switch documentation to JSDoc and Sphinx Switch documentation to JSDoc and Sphinx Oct 6, 2017
@galfert galfert merged commit 593b7d5 into master Oct 6, 2017
@galfert galfert deleted the docs/sphinx branch October 6, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Sync interval getter/setter functions hidden in internal API docs
3 participants