Issue 4970 - Document the library API of python-abp

Issue 4970 - Document the library API of python-abp

repository: https://hg.adblockplus.org/python-abp
based on this review: https://codereview.adblockplus.org/29465715/

[Vasily Kuznetsov, 2017-06-14 17:47:05]

This is the documentation for the APIs added by 4969 and some of the existing
APIs.

[Vasily Kuznetsov, 2017-07-26 14:53:51]

Hi Matze,

This is the documentation for the #4969 changes. Thanks in advance for your
feedback.

Cheers,
Vasily

[Vasily Kuznetsov, 2017-08-07 20:31:50]

Hi Matze,

I updated the README here to match the changes from the other review. See if
this makes sense and then I will land them together.

Cheers,
Vasily

P.S. BTW, here's a neat markdown previewer I found today:
https://github.com/joeyespo/grip
The idea is that it looks pretty much like github.

[mathias, 2017-08-08 12:24:35]

Honestly I think this patch-set is way too large; I don't really see the point
in listing all the different property types and values. It should be way more
straightforward to just briefly explain the general concept(s) in the README
file, and use the time saved on not repeating the enumerations to make sure the
help(...) function and/or other documentation generator is properly fed and
generates useful output. What do you think?

https://codereview.adblockplus.org/29465720/diff/29508848/README.md
File README.md (right):

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode6
README.md:6: adblocking software.
For an introduction that is a bit too much. How about making two or three short
sentences out of this?

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode124
README.md:124: error and the original line that failed parsing.
It is not clear what bits this is about (I assume the generator that is function
parse_filterlist) and the following section/list seems a bit unrelated.

Also, as a side-note, I am not sure this is the desired behavior - analogous to
EmptyLine there should be an InvalidLine or something that contains an .error
reference to the exception object and the .raw string. Otherwise one would
always abort processing a list just because of something being allegedly
malformed, which could very well be something one just wants to skip over and
log about.

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode131
README.md:131: If you find that this is bothering you, you probably want to
convert the output
Everything in this section from here on, maybe including the two items listed
above, seems terribly obvious after the first **Note:** earlier about
parse_filterlist() being a generator. Do you really think it is worth the effort
documenting that stuff in explicit fashion?

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode143
README.md:143: | type       | attributes                                        
                    |
Are you sure this kind of table markup is supported by all the systems we want
to properly render the README.md file? That'll be news to me, I was under the
impression that this is basically a part of the "GitHub flavor" just loosely
adopted by some libraries.

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode152
README.md:152: #### Filter atributes
This section mentions "Selector" but not ".selector", "Action" but not
".action", etc. - hence it's not obvious to the reader what to do with that
information and what it is about.

[Vasily Kuznetsov, 2017-08-08 14:31:12]

Hi Matze,

In general I think you're right. This starts turing into something like an API
documentation instead of a README file, but it kind of sucks at that too since
it's not structured consistently.

I will follow your advice and improve the `help(foo)` behavior as well as make
the README a TLDR version of the current content.

Cheers,
Vasily

https://codereview.adblockplus.org/29465720/diff/29508848/README.md
File README.md (right):

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode124
README.md:124: error and the original line that failed parsing.
On 2017/08/08 12:24:35, mathias wrote:
> It is not clear what bits this is about (I assume the generator that is
function
> parse_filterlist) and the following section/list seems a bit unrelated.
> 
> Also, as a side-note, I am not sure this is the desired behavior - analogous
to
> EmptyLine there should be an InvalidLine or something that contains an .error
> reference to the exception object and the .raw string. Otherwise one would
> always abort processing a list just because of something being allegedly
> malformed, which could very well be something one just wants to skip over and
> log about.

Yeah, we've discussed this. But for now that change is not in yet, so I'm
documenting the actual behavior.

https://codereview.adblockplus.org/29465720/diff/29508848/README.md#newcode143
README.md:143: | type       | attributes                                        
                    |
On 2017/08/08 12:24:35, mathias wrote:
> Are you sure this kind of table markup is supported by all the systems we want
> to properly render the README.md file? That'll be news to me, I was under the
> impression that this is basically a part of the "GitHub flavor" just loosely
> adopted by some libraries. 

Indeed the table markup was not part of the original Markdown. However, it does
seem to be supported by all websites, tools and libraries that I've seen.

[Vasily Kuznetsov, 2017-10-10 16:30:18]

Hi Matze,

At last I found some time to look at this review! I scaled down the README as
you proposed and made sure all public stuff has proper docstrings and works well
with `help()`. I've chosen to change the docstring convention to NumPy/SciPy
because it's human-readable, well documented and works with automation
(including automatic docstring validation). Seems like this is a better way to
go than the former docstrings that were not really human-friendly.

Let me know what you think.

Cheers,
Vasily

[mathias, 2017-10-24 15:24:36]

LGTM.

[Vasily Kuznetsov, 2017-10-24 16:11:00]

Rebased to current master and did a few cosmetic changes.

https://codereview.adblockplus.org/29465720/diff/29587695/abp/filters/parser.py
File abp/filters/parser.py (right):

https://codereview.adblockplus.org/29465720/diff/29587695/abp/filters/parser....
abp/filters/parser.py:42: 
pep8-docstrings demands an empty line at the end of numpy-style docstrings,
although it seems somewhat inconsistently. Anyway, they are more readable this
way.

https://codereview.adblockplus.org/29465720/diff/29587695/abp/filters/parser....
abp/filters/parser.py:313: TypeError
As suggested by Matze, this error is easy to foresee, so we might as well
document it.

https://codereview.adblockplus.org/29465720/diff/29587695/tests/test_render_s...
File tests/test_render_script.py (right):

https://codereview.adblockplus.org/29465720/diff/29587695/tests/test_render_s...
tests/test_render_script.py:32: except ImportError:  # The modules were
renamed/moved in Python 3.
flake8 started complaining about a bare except -- fair enough, I added
ImportError there.

[mathias, 2017-10-24 16:15:16]

LGTM.