Bug 1409654 - Review Request: python-pydocstyle - Python docstring style checker
Summary: Review Request: python-pydocstyle - Python docstring style checker
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Randy Barlow
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-02 19:40 UTC by Tadej Janež
Modified: 2017-07-13 01:22 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-07-07 23:06:15 UTC
randy: fedora-review+


Attachments (Terms of Use)
review.txt (10.47 KB, text/plain)
2017-06-02 19:48 UTC, Randy Barlow
no flags Details

Description Tadej Janež 2017-01-02 19:40:43 UTC
Spec URL: https://raw.githubusercontent.com/tjanez/pydocstyle-package/c6882b9/python-pydocstyle.spec
SRPM URL: https://tadej.fedorapeople.org/python-pydocstyle-1.1.1-0.fc26.1.src.rpm

Description:
A static analysis tool for checking compliance with Python docstring
conventions.

It supports most of PEP 257 out of the box, but it should not be considered a
reference implementation.


Fedora Account System Username: tadej

NOTE: Rpmlint reports 16 python-bytecode-wrong-magic-value errors (with Python 3.6) which is a bug in rpmlint that has been fixed in the latest rawhide version [1].
Besides that, Rpmlint reports 8 warnings, 6 of those are spelling errors which I decided to ignore since docstring is an established term in Python community. The remaining 2 warnings are about missing manual pages. I'll query upstream if they could provide one.

Trimmed Rpmlint output:
python2-pydocstyle.noarch: W: spelling-error Summary(en_US) docstring -> doc string, doc-string, stringing
python2-pydocstyle.noarch: W: spelling-error %description -l en_US docstring -> doc string, doc-string, stringing
python3-pydocstyle.noarch: W: spelling-error Summary(en_US) docstring -> doc string, doc-string, stringing
python3-pydocstyle.noarch: W: spelling-error %description -l en_US docstring -> doc string, doc-string, stringing
python3-pydocstyle.noarch: W: no-manual-page-for-binary pep257
python3-pydocstyle.noarch: W: no-manual-page-for-binary pydocstyle
python-pydocstyle.src: W: spelling-error Summary(en_US) docstring -> doc string, doc-string, stringing
python-pydocstyle.src: W: spelling-error %description -l en_US docstring -> doc string, doc-string, stringing

[1] https://koji.fedoraproject.org/koji/buildinfo?buildID=830119

Comment 1 Igor Gnatenko 2017-01-02 20:12:05 UTC
any reason not to take archive from github?

Not sure how it analyzing it, but I think that you should provide binaries for both py2 and py3.

Comment 2 Tadej Janež 2017-01-03 09:38:58 UTC
(In reply to Igor Gnatenko from comment #1)
> any reason not to take archive from github?

Hmm... I thought GitHub Releases page only contained git tags so I chose to use the release on PyPI to follow [1]:

"If the upstream does create tarballs you should use them as tarballs provide an easier trail for people auditing the packages."

I see now that the same zip source archive is also hosted on GitHub. I guess both, PyPI and GitHub, are equivalent now or you think GitHub should be preferred?

> Not sure how it analyzing it, but I think that you should provide binaries
> for both py2 and py3.

What are the arguments for providing binaries for both versions of Python?

The Packaging Guidelines for Python says:

"If the executables provide the same functionality independent of whether they are run on top of Python 2 or Python 3, then only the Python 3 version of the executable should be packaged." [2]

I think pydocstyle running on Python 3 will happily check Python 2 source code docstrings.

BTW, are you volunteering for a review ;-)?

[1] https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services
[2] https://fedoraproject.org/wiki/Packaging:Python#Executables_in_.2Fusr.2Fbin

Comment 3 Igor Gnatenko 2017-01-03 09:53:51 UTC
(In reply to Tadej Janež from comment #2)
> (In reply to Igor Gnatenko from comment #1)
> > any reason not to take archive from github?
> 
> Hmm... I thought GitHub Releases page only contained git tags so I chose to
> use the release on PyPI to follow [1]:
> 
> "If the upstream does create tarballs you should use them as tarballs
> provide an easier trail for people auditing the packages."
> 
> I see now that the same zip source archive is also hosted on GitHub. I guess
> both, PyPI and GitHub, are equivalent now or you think GitHub should be
> preferred?
GitHub should be preferred as it's *real* source (with licenses, tests and all stuff). PyPI tarball is pre-processed source (usually without license, tests).
> 
> > Not sure how it analyzing it, but I think that you should provide binaries
> > for both py2 and py3.
> 
> What are the arguments for providing binaries for both versions of Python?
> 
> The Packaging Guidelines for Python says:
> 
> "If the executables provide the same functionality independent of whether
> they are run on top of Python 2 or Python 3, then only the Python 3 version
> of the executable should be packaged." [2]
> 
> I think pydocstyle running on Python 3 will happily check Python 2 source
> code docstrings.
Question whether if it will check py2 source code docstrings properly. E.g. you have py3-incompatible source.
> 
> BTW, are you volunteering for a review ;-)?
If you are going to change source to github - then definitely yes ;)
> 
> [1] https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services
> [2]
> https://fedoraproject.org/wiki/Packaging:Python#Executables_in_.2Fusr.2Fbin

Comment 4 Tadej Janež 2017-01-05 16:02:27 UTC
(In reply to Igor Gnatenko from comment #3)
> (In reply to Tadej Janež from comment #2)
> GitHub should be preferred as it's *real* source (with licenses, tests and
> all stuff). PyPI tarball is pre-processed source (usually without license,
> tests).

I disagree. The PyPI tarball is the actual upstream release tarball, whereas the GitHub's Source archive link [1] is just a tarball of everything that is in the git repo.
(As a side-note, pydocstyle actually uses GitHub's Releases feature and they provide the same release tarball there [2] as is available on PyPI [3].)

I'm sure you know that in the Python world, source distributions are usually made with 'python setup.py sdist' command. And application authors can control what gets included in the source distribution by specifying it in setup.py and MANIFEST.in files.

In pydocstyle's case, the authors decided to include the LICENSE file in the source distribution (LICENSE-MIT) but they omitted the tests (src/tests). I think tests are valuable and should be included in the source distribution.
However, I think I should try to persuade the upstream to include them, not side-step them by using the GitHub's Source archive link.

> Question whether if it will check py2 source code docstrings properly. E.g.
> you have py3-incompatible source.

I checked this with Django 1.4 which is a quite large Python 2 project without Python 3 support and it works without a problem.

Here are the details:

[tadej@tlinux64 pydocstyle-for-python2-only-code]$ wget -q https://www.djangoproject.com/download/1.4/tarball -O Django-1.4.tar.gz
[tadej@tlinux64 pydocstyle-for-python2-only-code]$ tar -xf Django-1.4.tar.gz 
[tadej@tlinux64 pydocstyle-for-python2-only-code]$ cd Django-1.4/
[tadej@tlinux64 Django-1.4]$ cat $(which pydocstyle)
#!/usr/bin/python3
# EASY-INSTALL-ENTRY-SCRIPT: 'pydocstyle==1.1.1','console_scripts','pydocstyle'
__requires__ = 'pydocstyle==1.1.1'
import sys
from pkg_resources import load_entry_point

if __name__ == '__main__':
    sys.exit(
        load_entry_point('pydocstyle==1.1.1', 'console_scripts', 'pydocstyle')()
    )
[tadej@tlinux64 Django-1.4]$ pydocstyle django
django/contrib/admin/validation.py WARNING: __all__ is defined as a list, this means pydocstyle cannot reliably detect contents of the __all__ variable, because it can be mutated. Change __all__ to be an (immutable) tuple, to remove this warning. Note, pydocstyle uses __all__ to detect which definitions are public, to warn if public definitions are missing docstrings. If __all__ is a (mutable) list, pydocstyle cannot reliably assume its contents. pydocstyle will proceed assuming __all__ is not mutated.
django/contrib/gis/measure.py WARNING: __all__ is defined as a list, this means pydocstyle cannot reliably detect contents of the __all__ variable, because it can be mutated. Change __all__ to be an (immutable) tuple, to remove this warning. Note, pydocstyle uses __all__ to detect which definitions are public, to warn if public definitions are missing docstrings. If __all__ is a (mutable) list, pydocstyle cannot reliably assume its contents. pydocstyle will proceed assuming __all__ is not mutated.
[ ... output trimmed ... ]
django/__init__.py:1 at module level:
        D104: Missing docstring in public package
django/__init__.py:3 in public function `get_version`:
        D401: First line should be in imperative mood ('Derive', not 'Derives')
django/bin/django-admin.py:1 at module level:
        D100: Missing docstring in public module
[ ...  output trimmed ... ]
django/middleware/common.py:148 in private function `_is_internal_request`:
        D401: First line should be in imperative mood ('Return', not 'Returns')
django/middleware/__init__.py:1 at module level:
        D104: Missing docstring in public package
[tadej@tlinux64 Django-1.4]$

> > 
> > BTW, are you volunteering for a review ;-)?
> If you are going to change source to github - then definitely yes ;)

I can change it to https://github.com/PyCQA/pydocstyle/releases/download/1.1.1/pydocstyle-1.1.1.zip :-)


[1] https://github.com/PyCQA/pydocstyle/archive/1.1.1.tar.gz
[2] https://github.com/PyCQA/pydocstyle/releases/download/1.1.1/pydocstyle-1.1.1.zip
[3] https://files.pythonhosted.org/packages/source/p/pydocstyle/pydocstyle-1.1.1.zip

Comment 5 fszymanski 2017-01-11 16:18:53 UTC
Hi,

In this case I have to agree with Igor, it would be better to use the GitHub source. It contains tests and all files have Unix line endings (conversion unnecessary). Use the tarball and not the zip archive.
Correct source URL:
Source0:  https://github.com/PyCQA/%{srcname}/archive/%{version}/%{srcname}-%{version}.tar.gz

Things like removing shebang from a file or dos2unix should be done in the %prep section.

What's up with the Release tag (0%{?dist}.1)?
Should be:
Release:  1%{?dist}

Comment 6 Randy Barlow 2017-03-17 18:42:37 UTC
I don't have strong feelings about which source is used, though I do think using the GitHub link is better since it will give you the tests and those can be handy in Koschei.

However, I also don't want to override Igor or fszymanski. I would like to use this package, so if you are willing to use the GitHub link to satisfy them I'll do the review for you.

Comment 7 Tadej Janež 2017-04-07 09:18:40 UTC
Igor, Randy,

I've changed the source to GitHub archive download service until upstream includes tests in the release tarballs. I'll submit a pull request for that in near future.

With that I was able to enable tests in %check (with some minor source code modifications).

Regarding fszymanski comment about the release tag, I prefer to use 0%{?dist}.1, 0%{?dist}.2, ... during review phase. Of course, I will change it to 1%{?dist} after the package is accepted and commited into dist-git.

Here is the updated package:
Spec URL: https://raw.githubusercontent.com/tjanez/pydocstyle-package/3456fe8/python-pydocstyle.spec
SRPM URL: https://tadej.fedorapeople.org/python-pydocstyle-1.1.1-0.fc27.2.src.rpm

I would be very happy if any of you do the review.

Comment 8 Tadej Janež 2017-04-26 10:59:20 UTC
(In reply to Tadej Janež from comment #7)
> 
> I've changed the source to GitHub archive download service until upstream
> includes tests in the release tarballs. I'll submit a pull request for that
> in near future.

Pull request to include documentation and tests in source distribution has been submitted: https://github.com/PyCQA/pydocstyle/pull/252

I also wanted to update pydocstyle to version 2.0.0 but encountered an issue where the git tag for version 2.0.0 references different code than the code released on PyPI: https://github.com/PyCQA/pydocstyle/issues/251.

So, this affirms my concerns that GitHub tags are not the same as *official* releases on PyPI.

Igor, Randy,

I would be very happy if any of you could do the review.

Comment 9 Tadej Janež 2017-05-08 20:46:11 UTC
Igor, Randy,

I've updated pydocstyle to version 2.0.0.

The issue where the git tag for version 2.0.0 referenced different code than the code released on PyPI (https://github.com/PyCQA/pydocstyle/issues/251) was resolved by upstream.

Here is the updated package:
Spec URL: https://raw.githubusercontent.com/tjanez/pydocstyle-package/0e49514/python-pydocstyle.spec
SRPM URL: https://tadej.fedorapeople.org/python-pydocstyle-2.0.0-0.fc27.1.src.rpm

I would be very happy if any of you would do the review.

Comment 10 Randy Barlow 2017-06-02 19:48:54 UTC
Created attachment 1284508 [details]
review.txt

Comment 11 David H. Gutteridge 2017-06-20 02:30:35 UTC
Just adding myself to the CC list as an interested user, as I've found pydocstyle handy in the past, and I appreciate the effort to add it to Fedora.

Comment 12 Randy Barlow 2017-06-27 22:13:34 UTC
Hello!

This is just a reminder that this package is approved and ready to be added to Fedora. Thanks!

Comment 13 Tadej Janež 2017-06-28 19:44:13 UTC
Hi Randy,

(In reply to Randy Barlow from comment #12)
> 
> This is just a reminder that this package is approved and ready to be added
> to Fedora. Thanks!

thanks for the review and the reminder!

I'll add this to Fedora ASAP.

Comment 14 Gwyn Ciesla 2017-06-29 11:47:47 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/python-pydocstyle

Comment 15 Fedora Update System 2017-06-30 12:51:31 UTC
python-pydocstyle-2.0.0-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-2b1a47a1ae

Comment 16 Fedora Update System 2017-06-30 20:26:48 UTC
python-pydocstyle-2.0.0-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-2b1a47a1ae

Comment 17 Fedora Update System 2017-07-02 03:53:13 UTC
python-pydocstyle-2.0.0-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-4db0e57f0b

Comment 18 Fedora Update System 2017-07-07 23:06:15 UTC
python-pydocstyle-2.0.0-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 19 Fedora Update System 2017-07-13 01:22:37 UTC
python-pydocstyle-2.0.0-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.