Skip to content

Conversation

@bastibe
Copy link
Owner

@bastibe bastibe commented Sep 21, 2014

  • translated all docstrings from Markdown to ReST
  • added trivial documentation configuration

- translated all docstrings from Markdown to ReST
- added trivial documentation configuration
@bastibe
Copy link
Owner Author

bastibe commented Sep 21, 2014

This is very much work in progress and not meant for merging yet.

- all parameters are documented
- all return values are documented
- every function and method has an example
we still have to add more information though
@bastibe
Copy link
Owner Author

bastibe commented Sep 27, 2014

This still needs a bit of work. In particular, the README should be expanded, and we should select a more beautiful theme. Apart from that, this should be a workable prototype though.

README.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually much easier than that!
'w' is implied and channels is automatically taken from data.
Also, data.shape[1] doesn't work on one-dimensional data.

Copy link
Owner Author

Choose a reason for hiding this comment

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

true

@mgeier
Copy link
Contributor

mgeier commented Oct 14, 2014

That's much more than a prototype, that's great!

I have a few comments/questions though:

I think the files somehow clutter the main directory. Especially having a Makefile there may be a bit confusing.
What about putting all this into a subdirectory, e.g. doc/?

I (as so often) don't like the amount of repetition.
E.g., file, samplerate, channels, subtype, endian is documented multiple times.
This makes it really hard (and annoying) to maintain.
And I think it's even annoying to read, because when you scroll through you see the same stuff over and over again.

I don't like naming (single) return arguments.
I think this only makes sense when returning tuples.

Lower camelCase is a no-go in Python (except probably for compatibility with external non-Pythonic APIs), please don't use fObj and soundFile.

Some possible options for conf.py:

To show constructor docs (I like this much better than documenting __init__(), which looks quite strange): autoclass_content = "init"
To use the same order as in the source: autodoc_member_order = "bysource"
Nicer return type annotation: napoleon_use_rtype = False

@mgeier
Copy link
Contributor

mgeier commented Oct 15, 2014

You should also move the module docstring to the top of the file (see https://github.com/bastibe/PySoundFile/issues/57#issuecomment-49043385).
Otherwise it's not recognized by automodule.

Also, I think there is again too much repetition in the module docstring.
I think it should be only a few lines, containing mainly a few links, especially a link to the readthedocs page.

README.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should be import pysoundfile as sf

@mgeier
Copy link
Contributor

mgeier commented Oct 15, 2014

Is it intentional that you write :func:pysoundfile.SoundFile.write within the class documentation? It seems overly verbose to me. What about `:meth:`.write?

I think it's strange though, that Sphinx seems to require the dot, otherwise the link points to the write() function instead of the method.

Also in case of the function references, the explicit pysoundfile. seems superfluous to me (e.g. :func:pysoundfile.available_formats``).

pysoundfile.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the above paragraph is redundant because it's repeated below in the parameter list.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it is a nice summary. It's always nice to have a more readable introduction and example. We could probably get rid of it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is indeed a nice summary with nice examples.

I guess I was mainly disturbed by the listing of the endian-nesses, which is repeated below (and not that important).

What about this?

        * an *endian-ness*, which doesn't have to be specified at all in
          most cases (because the default ``'FILE'`` should do fine).

Maybe the thing in parentheses is not even necessary.

BTW, we should mention default_subtype() in the "Parameters" section!

@mgeier
Copy link
Contributor

mgeier commented Oct 22, 2014

I added a few commits as discussed.
I'm not ready, though, more commits will be coming in the next few days ...

The current path is needed for the mock modules.
The parent path is needed to actually enable the "show source" option
on readthedocs.org (everything else works fine without it ...).
@mgeier mgeier changed the title naive Sphinx documentation Sphinx documentation Oct 26, 2014
@mgeier mgeier modified the milestone: 0.6.0 Oct 26, 2014
@mgeier
Copy link
Contributor

mgeier commented Oct 27, 2014

OK, I think I'm done for now ...
The preliminary results can be seen on http://pysoundfile.rtfd.org/en/documentation/.

@mgeier
Copy link
Contributor

mgeier commented Nov 13, 2014

I just saw that in __init__() we write that endian is sometimes optional and that it is obtained from the file except for 'RAW' files.

This sounds totally reasonable to me.

The problem is, in libsndfile this is actually always optional, even for 'RAW' files.
The default value is always 'FILE'.
What does 'FILE' mean for 'RAW' files?
Is this a bug in libsndfile?
Is this documented somewhere (I didn't find it ...)?
Or am I missing something?

@bastibe
Copy link
Owner Author

bastibe commented Nov 13, 2014

This is absolutely terrific! I love the new documentation!

Regarding endian-ness: I don't know. Presumably, RAW files would use the machine convention if no endian-ness is given.

@mgeier
Copy link
Contributor

mgeier commented Nov 15, 2014

I'm also quite content with the new docs!
The README could get some more work, but we can do that separately, according to issue #26.
I would like to try to make the doctests actually pass, but I think we can also do that later.

The RAW-thing is a bit strange, but I think it's best if we leave our current description, because users should actually specify an endian-ness for RAW files, even if they don't have to according to libsndfile.

Is there anything I should change or are we merge-ready?

@mgeier
Copy link
Contributor

mgeier commented Nov 15, 2014

BTW, when merging, we'll have to change the documentation of seek(), because its behavior was changed in #67.

Instead of returning -1, after merging it will raise an exception. So this sentence in the "Returns" section:

The new absolute read/write position in frames, or a negative value on error.

Should be changed to:

The new absolute read/write position in frames.

I don't think we have to explicitly mention the exception.

@bastibe
Copy link
Owner Author

bastibe commented Nov 16, 2014

Let's merge!

@mgeier mgeier merged commit 855d6cb into master Nov 16, 2014
mgeier added a commit that referenced this pull request Nov 16, 2014
Pull Request #74.

Conflicts:
	README.md
	pysoundfile.py
	tests/test_pysoundfile.py
@mgeier mgeier deleted the documentation branch November 16, 2014 14:52
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