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

Introduce documentation built with sphinx #48

Merged
merged 19 commits into from
May 12, 2021
Merged

Introduce documentation built with sphinx #48

merged 19 commits into from
May 12, 2021

Conversation

JOJ0
Copy link
Contributor

@JOJ0 JOJ0 commented Apr 29, 2021

Starting to document using the wonderful possibilities of Sphinx

Main intentions:

  • Move as much as possible of existing documentation in to nicely rendered html pages hosted on python3-discogs-client.readthedocs.org
  • Leave as little as necessary on README.mkd to give the user a quick idea of how things will work when using p3dc and certainly link nicely to the readthedocs pages (currently links not working since "latest" builld is not available yet, will be after merge)
  • Have a look at a rendered version of the docs from dev branch sphinx_docs: https://python3-discogs-client.readthedocs.io/en/sphinx_docs/index.html
  • Keep markdown the main documentation language
  • include autogenerated documentation of the modules in discogs_client package

Todo:

  • discogs_client package documentation could be improvied by providing params/types in docstring
    • I suggest using google docstrings format - better human readable and vertically shorter than classic sphinx format -> let's use numpy style
    • Not with this PR - I think it already is ok and helpful, will do another PR -> we will verbosly document methods/classes if specific questions are raised by user's
  • Read through Readme and documentation and make sure everything makes sense throughout the documentation - e.g things like discogs client object called "ds" in one place and "d" in another need to be fixed.
  • When heading from README.mkdi into the linked read the docs sections. Things should still make sense withouthaving to go into all the details already descibed in other/previous sections
  • Some example code is using a repl, other parts is not - decide on which way to go and keep in mind that copy/pasting of repl examples is tedious (>>>>) -> decided to keep REPL (instead of rewriting) in README.mkd and in fetching_data.md
  • decide which of these URL options we want:
    • python3-discogs-client.readthedocs.org (abbreviated form: python3-discgos-client.rtd.org)
    • discogs-client.readthedocs.org (abbreviated form: discgos-client.rtd.org, name reflecting package name more or less, _ is -)
  • add a logo to top of sidebar

For more details on the whys and hows have a look at hist commit's message: b332c77

- use existing docs/ dir as the sphinx root directory
- move existing markdown files in docs/ to docs/source
- README.mkd should only contain brief examples
- move longish stuff from README.mkd to separate markdown files in docs/source
- sphinx doctree show those markdown files - let's call them "sections"
- add a sphinx_requirements.txt containing both the discogs_client deps
  and necessary sphinx packages
  - installation of discogs_client using setup.py is necessary when
    building docs locally or via readthedocs.org
  - only one requirements file is configurable, hence the need of
    doubling actual discogs_client (and non-sphinx) requirements into one file
@JOJ0
Copy link
Contributor Author

JOJ0 commented Apr 29, 2021

@AnssiAhola since I moved around and chopped up your nicely written help section about the Listing features, I'd like to kindly ask your opinion. Without furter words, have a look at the readme page in sphinx_doc branch as a starting point. Hit the link to the detailed docs on readthedocs (link on readme broken due to above described obvious reasons, use this one: https://python3-discogs-client.readthedocs.io/en/sphinx_docs/listing.html)

@AnssiAhola
Copy link
Contributor

AnssiAhola commented Apr 29, 2021

Looks nice, I would maybe edit the headers and code examples like so

Add

from discogs_client import Client, Condition, Status, Sort

d = Client('user-agent', user_token='my_user_token')
me = d.identity()

me.inventory.add_listing(
    release=15246519,                       # Also accepts an Release object
    condition=Condition.MINT,               # condition set to 'Mint (M)'
    price=29.99,
    status=Status.DRAFT,                    # status set to 'Draft'
    sleeve_condition=Condition.NEAR_MINT    # sleeve condition set to 'Near Mint (NM or M-)'
)

Update

For example, get the most expensive listing and update its price.

# No changes in code

Delete

Instantiate a listing object as descibed described in the previous example and call

# No changes in code

to remove it.

Not sure if necessary, but maybe add an explanation about the Condition etc enums. In here or link to different page, your call 😄

@alifhughes
Copy link
Contributor

@JOJ0 These docs are amazing! Really really useful and will be great to continually add to these. I think I agree with all your points:

  • We should keep readme quite basic and refer users to these docs
  • Markdown is a yes from me
  • And yes, documenting the code so that it automatically appears in the docs would also be nice to prevent 'what functions can I do on which objects'

What is the scope of this PR? Do we want to do some of these TODOs or do we think we're ready with this 'as is'? Would you like some help documenting stuff?
Great work!

@JOJ0
Copy link
Contributor Author

JOJ0 commented May 2, 2021

I think I could finish a first version that overall makes sense to the reader on my own. That would be the goal. But many thanks for offering help! That would be needed with later PR's that document docstrings better. I will include some examples of google style docstrings how I think they should roughly look like. Also I will investigate how some of the already present docstrings of former coders look like and see if it makes sense to continue writing new ones similarily.

One thing I'd like you opinion: Happy with the current readthedocs url or the other option?

@alifhughes
Copy link
Contributor

One thing I'd like you opinion: Happy with the current readthedocs url or the other option?

Tbh I quite like the 'readthedocs' url, it is explicit and reads well as a whole 🙂

@JOJ0
Copy link
Contributor Author

JOJ0 commented May 2, 2021

Looks nice, I would maybe edit the headers and code examples like so

added a commit including your suggestions.

Not sure if necessary, but maybe add an explanation about the Condition etc enums. In here or link to different page, your call 😄

yes, good idea, I did that by linking directly to the enum classes. Have a look: https://python3-discogs-client.readthedocs.io/en/sphinx_docs/listing.html

Suggestions always welcome! Thanks a lot for the help!

@AnssiAhola
Copy link
Contributor

Seems like the Sort enum isn't showing the Sort.By and Sort.Order enums properly, maybe because they're nested 🤔

- all instances of the client renamed from ds. to d.
- add links to corresponding modules/classes
- replace repl examples with regular code examples
- all instances of the client called d.
- rephrase a few sentences
- FIXME this has to be read through carefully and possibly rephrased in
  some more places!
@JOJ0
Copy link
Contributor Author

JOJ0 commented May 3, 2021

Seems like the Sort enum isn't showing the Sort.By and Sort.Order enums properly, maybe because they're nested 🤔

ah interesting, thanks for the hint. will look into that!

@JOJ0
Copy link
Contributor Author

JOJ0 commented May 3, 2021

Alright, added some commits today @alifhughes

One thing I could need your opinion with is: We now have two places where authentication methods are described. in readme.mkd and in authentication.md - Can't decide which version is better.

My goal is to have the easy/shorter one (token) on the readme directly and both of them in more detail on authenticaition.md

Which version I should get rid of? but probably I need to combine them anyway, because there is useful stuff in both of them. just too lazy for now as I never did OAuth, so need to test first and then see which version is the most accurate.

Another thing I started today is: Try to use one style of examples - currently we have repl examples and "regular code" examples. Or another option could be to use repl examples on readme.md but then when clicking into the docs, all examples are regular ones - Help me decided! What I don't like is the tedious copy/pasting of the repl examples on readme.mkd (and why the hell was this weird extension for .md chosen btw?)

Also realized today that the documents that where buried in docs dir (authentication.md, quickstart.md) don't use the best english and are written clumsy in some places. I started to rephrase already but not through with it.

- improve phrasing
- test examples
- provide more detail, especially on where to find consumer key & secret
  and what exactely providing callback_url does
- replace prevsiously used term "Discogs auth" with "User-token
  authentication"
@JOJ0
Copy link
Contributor Author

JOJ0 commented May 6, 2021

Reworked authentication.md again. By any chance you find the time to work through it @alifhughes, trying out the examples would be helpful.

I rephrased a lot and provide more detail after I really tested it all through - First time OAuth with discogs_client for me actually ;-)

Give it a try but if too tedious and no spare time, don't worry, even if there might be room for improvement I think it's fine and understandable - or let's rather put it: "Works for me" :-P

PS: Didn't remove the OAuth chapter form readme.mkd yet - left it for reference, but in the end I think it's sufficient if we just provide the quick method of user-token auth on readme and point to sphinx docs for OAuth. Agree?

@alifhughes
Copy link
Contributor

Reworked authentication.md again. By any chance you find the time to work through it @alifhughes, trying out the examples would be helpful.

I rephrased a lot and provide more detail after I really tested it all through - First time OAuth with discogs_client for me actually ;-)

Give it a try but if too tedious and no spare time, don't worry, even if there might be room for improvement I think it's fine and understandable - or let's rather put it: "Works for me" :-P

PS: Didn't remove the OAuth chapter form readme.mkd yet - left it for reference, but in the end I think it's sufficient if we just provide the quick method of user-token auth on readme and point to sphinx docs for OAuth. Agree?

Hey @JOJ0 thanks for doing this! Will find the time tonight to go through the docs and test out the examples 🙂

- remove oauth section from readme, just link there
- all pointers to docs in italics and bold
- tiny fixes, links and pointers in all files (need to be checked in
  rendered version!!!)
- fix text width to 80 chars in all files
@JOJ0 JOJ0 force-pushed the sphinx_docs branch 5 times, most recently from 01711f7 to 733b33d Compare May 7, 2021 22:05
@JOJ0 JOJ0 force-pushed the sphinx_docs branch 8 times, most recently from e64d67a to 2aa389a Compare May 9, 2021 08:36
discogs_client.rst:
- show private (_) members
- experiment with nested enums not shown for utils.Sort.By

some options in conf.py:
- disable docstring inheritance (get rid of stating the same bollocks
  all over again on every child class)
- sort order as in source (not alphabetically)
- show docstring of class AND __init__ method
@JOJ0 JOJ0 marked this pull request as ready for review May 11, 2021 05:29
@JOJ0 JOJ0 force-pushed the sphinx_docs branch 4 times, most recently from 78b2ac1 to 378af98 Compare May 11, 2021 06:04
@JOJ0 JOJ0 force-pushed the sphinx_docs branch 2 times, most recently from 28a16ca to 0d6daa5 Compare May 11, 2021 07:15
and some fixes/reordering in README
JOJ0 added 2 commits May 11, 2021 09:32
- decide to use numpy style
- fix some class docstrings
- replace existing baseclass docstring (googly style) with numpy version
- remove Marketplace listing enums experiment in discogs_client.rst
  (unfortunately the issue with nested enum classes is not fixed)
@JOJ0
Copy link
Contributor Author

JOJ0 commented May 12, 2021

Let's get this one merged @alifhughes :-) Press the button!

Copy link
Contributor

@alifhughes alifhughes left a comment

Choose a reason for hiding this comment

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

Great work mate, really appreciate it 🎉

@JOJ0 JOJ0 merged commit d0a8ae8 into master May 12, 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.

3 participants