piskvorky / gensim

Topic Modelling for Humans
https://radimrehurek.com/gensim
GNU Lesser General Public License v2.1
15.71k stars 4.38k forks source link

Add style checking for docstrings #1421

Closed vlejd closed 5 years ago

vlejd commented 7 years ago

Description

Gensim does not have set up style checks for documentation to match format desired by #1411 .

Here are some options:

I think the best option is to add pylint to travis with correct .pylintrc . The problem is that using two style checkers seems a bit weird. Second problem is, that pylint does not have something like flake8 diff to compare errors in a diff. We would have to run something pylint <old_commit> -rn --load-plugins=pylint.extensions.docparams --accept-no-param-doc=no --disable=C0301 --accept-no-raise-doc=no --accept-no-return-doc=no --accept-no-yields-doc=no > old_errors ; head -n -2 old_errors > old_errors and pylint <new_commit> -rn --load-plugins=pylint.extensions.docparams --accept-no-param-doc=no --disable=C0301 --accept-no-raise-doc=no --accept-no-return-doc=no --accept-no-yields-doc=no > new_errors; head -n -2 new_errors > new_errors to get all errors in the last stage and in the new stage. Then run diff -u old_errors new_errors | grep -E "^\+" to filter only errors that were added.

vlejd commented 7 years ago

@menshikh-iv Any thoughts?

rasto2211 commented 7 years ago

@tmylk opened a similar issue couple months ago (issue #1192). Sphinx runs also some checks on docstrings. The sad thing about gensim documentation is that it throws over 100 warnings at the compilation. @souravsingh made PR which got rid of couple warning but it seems that since that time new code brought additional warnings. When I try to compile the documentation I see 112 warnings. Maybe it is also a good idea to add a travis job which compiles the documentation and checks if the new code does not bring new errors and warnings. Maybe just a simple diff is sufficient.

menshikh-iv commented 7 years ago

@vlejd @rasto2211 I think we can add building documentation step to travis (but before we need to fix current errors).

I think doc-checker from pylint will be a good option, but if we will be using it, do we need to run Sphinx build to Travis?

rasto2211 commented 7 years ago

Well, I don't see a reason why it is a bad idea.

menshikh-iv commented 7 years ago

The question is whether we add both variants (Sphinx build and pylint doc-check) to travis or pylint only.

rasto2211 commented 7 years ago

We still need to use sphinx-build to build the docs so I think it is a good idea to make sure that we don't introduce additional warnings in new commits. I don't think that sphinx checks are subset of pylint checks. Besides that, make html takes only 15 sec. on my machine. I don't think that additional 15 sec. on travis would hurt. Currently, the total build time is around 53 minutes.

menshikh-iv commented 7 years ago

@rasto2211 I agree with you, but we will give errors from all documentation from Sphinx every time. I don't want that Sphinx break Travis every time. As a zero step, we need to fix Sphinx errors.

mpenkov commented 5 years ago

Why can't we use doctest for this?

menshikh-iv commented 5 years ago

I think I can close this, because now

So, next step for improve doc & automatization is #2107