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

D102 Missing docstring in public method #1963

Closed
peterjc opened this issue Mar 15, 2019 · 35 comments
Closed

D102 Missing docstring in public method #1963

peterjc opened this issue Mar 15, 2019 · 35 comments
Labels

Comments

@peterjc
Copy link
Member

peterjc commented Mar 15, 2019

We currently enforce various code style issues using flake8, configured via Bio/.flake8 etc and run automatically on TravisCI via tox. More details here including how to install flake8 and the relevant plugins:

https://github.com/biopython/biopython/blob/master/CONTRIBUTING.rst

One of the important docstring checks we currently do not enforce is missing docstrings in methods of public classes.

This issue to to add the remaining missing method docstrings, and then finally turn on the D102 checks by removing them from the ignore list here:

https://github.com/biopython/biopython/blob/master/Bio/.flake8

Currently there are 117 of these remaining following work on issue #1203 etc:

$ flake8 Bio --isolated --select D102 --statistics --quiet --quiet
117   D102 Missing docstring in public method
$ flake8 Bio --isolated --select D102 | sort
Bio/Align/AlignInfo.py:678:1: D102 Missing docstring in public method
Bio/Align/__init__.py:1335:1: D102 Missing docstring in public method
Bio/Align/__init__.py:1342:1: D102 Missing docstring in public method
Bio/AlignIO/EmbossIO.py:32:1: D102 Missing docstring in public method
Bio/AlignIO/EmbossIO.py:42:1: D102 Missing docstring in public method
...

This is fairly simple to fix, so I've tagged is as a "good first issue" and would particularly like to encourage new contributors to try working on this (although code authors are of course also welcome to fix their own omissions).

If you want to volunteer and work on a section, please comment here first to avoid duplication of effort. e.g. "I'll work on Bio/Wise/"

If you want to work on this and #1960 and/or #1961 together, that's fine too.

@michiboo
Copy link
Contributor

Hi I would like to work on this issue!

@MarkusPiotrowski
Copy link
Contributor

MarkusPiotrowski commented Mar 21, 2019

@michiboo Please select a module on which you would like to work on and tell us. Maybe as starter one that has not to many missing docstrings? E.g. Bio\Align?

If you are not experienced with doscstrings, I would suggest to read PEP 257 beforehand: https://www.python.org/dev/peps/pep-0257/

@michiboo
Copy link
Contributor

Yes I would work on this module Bio\Align for start!

@MarkusPiotrowski
Copy link
Contributor

MarkusPiotrowski commented Mar 21, 2019

Overview: Missing docstring in public method (D102)

Module Missing docstrings
Bio\Align_init_.py 2
Bio\Align\AlignInfo.py 1
Bio\AlignIO\EmbossIO.py 2
Bio\Blast\ParseBlastTable.py 1
Bio\Blast\NCBIXML.py 1
Bio\Blast\Record.py 1
Bio\codonalign\codonalignment.py 1
Bio\codonalign\codonseq.py 3
Bio\Crystal_init_.py 18
Bio\Entrez\Parser.py 26
Bio\ExPASy\ScanProsite.py 4
Bio\FSSP_init_.py 8
Bio\Nexus\Nexus.py 7
Bio\Nexus\Trees.py 1
Bio\PopGen\GenePop\Controller.py 12
Bio\PopGen\GenePop\EasyController.py 9
Bio\PopGen\GenePop\LargeFileParser.py 1
Bio\SCOP\Raf.py 1
Bio\SearchIO_legacy\ParserSupport.py 5
Bio\SeqIO_index.py 3
Bio\SeqIO\Interfaces.py 2
Bio\SeqIO\SffIO.py 2
Bio\Wise\dnal.py 1
Bio\Wise\psw.py 2

+/- 1, I counted the output of flake8 manually ;-)

Thank you, we are making progress!

$ flake8 Bio --isolated --select D102 --statistics --quiet --quiet
87    D102 Missing docstring in public method

@svalqui
Copy link
Contributor

svalqui commented Apr 1, 2019

I'll work on Bio.SeqIO.

@svalqui
Copy link
Contributor

svalqui commented Apr 3, 2019

I'll work on Bio.Crystal.

@svalqui
Copy link
Contributor

svalqui commented Apr 7, 2019

I'll work on Bio.Wise.

@svalqui
Copy link
Contributor

svalqui commented Apr 14, 2019

@peterjc , about Bio.SearchIO._legacy.ParserSupport.py, is commented as deprecated, would like the DocStrings updated regardless or would you like _legacy removed?

@svalqui
Copy link
Contributor

svalqui commented Apr 15, 2019

I'll work on Bio.SCOP.Raf.py

@peterjc
Copy link
Member Author

peterjc commented Apr 15, 2019

We still have Bio.SearchIO._legacy.ParserSupport.py in order to offer plain text BLAST parsing in SearchIO, although neither we nor the NCBI encourage using the human readable format this way. So while it is deprecated code, it is likely to hang on for a little while longer. We might as well add the missing docstrings - it wouldn't be much more work than adding # noqa: D102 flags.

peterjc pushed a commit that referenced this issue Apr 15, 2019
* Added missing docstrings (see github issue #1963)
* URL updated
* Changed alphameric to alphanumeric
@svalqui
Copy link
Contributor

svalqui commented Apr 15, 2019

I'll work on Bio.SearchIO._legacy

@svalqui
Copy link
Contributor

svalqui commented Apr 16, 2019

I'll work on Bio.PopGen

@peterjc
Copy link
Member Author

peterjc commented Apr 23, 2019

Excellent progress, thank you everyone so far 👍

$ flake8 Bio --isolated --select D102 --statistics --quiet --quiet
74    D102 Missing docstring in public method

Only eight files left under Bio,

$ flake8 Bio --isolated --select D102 | cut -d ":" -f 1 | sort | uniq
Bio/Entrez/Parser.py
Bio/ExPASy/ScanProsite.py
Bio/FSSP/__init__.py
Bio/Nexus/Nexus.py
Bio/Nexus/Trees.py
Bio/PopGen/GenePop/Controller.py
Bio/PopGen/GenePop/EasyController.py
Bio/PopGen/GenePop/LargeFileParser.py

@svalqui
Copy link
Contributor

svalqui commented Apr 23, 2019

I'll work on Bio.Nexus.

@peterjc
Copy link
Member Author

peterjc commented Apr 29, 2019

@Ikercasillas please beware that like TogoWS, this is an online module, and things like the search results from esearch will change over time.

Updated (I was thinking of Bio/Entrez/__init__.py rather than Bio/Entrez/Parser.py originally)

For this issue while doctests would be nice, they are not the priority.

@svalqui
Copy link
Contributor

svalqui commented May 2, 2019

I'll work on Bio.FSSP

@svalqui
Copy link
Contributor

svalqui commented May 14, 2019

I'll work on Bio.ExPASy

@svalqui
Copy link
Contributor

svalqui commented May 14, 2019

Note: Test/test_FSSP.py doen't use unittest, maybe should be part of #1288.

svalqui added a commit to svalqui/biopython that referenced this issue May 14, 2019
* Added missing docstrings (see github issue biopython#1963)
* URL updated
* Changed alphameric to alphanumeric
@peterjc
Copy link
Member Author

peterjc commented May 15, 2019

Excellent progress,

$ flake8 Bio --isolated --select D102 | cut -d ":" -f 1 | sort | uniq
Bio/Blast/NCBIXML.py
Bio/Blast/Record.py
Bio/Entrez/Parser.py
Bio/ExPASy/ScanProsite.py
Bio/Nexus/Nexus.py
Bio/Nexus/Trees.py

@svalqui
Copy link
Contributor

svalqui commented May 20, 2019

I'll work on Bio.Blast

@MarkusPiotrowski
Copy link
Contributor

If you look in one of the former D102 lists, you will see that the two Bio.Blast D102 issues are new. We should have a closer look at the PRs, otherwise, this will turn into a Sisyphean task

@peterjc
Copy link
Member Author

peterjc commented May 21, 2019

True:

$ git checkout biopython-173 && flake8 --isolated --select D102 Bio/Blast/
HEAD is now at 5ee5e69e6 Call this Biopython 1.73
Bio/Blast/ParseBlastTable.py:43:1: D102 Missing docstring in public method

vs:

$ git checkout master && flake8 --isolated --select D102 Bio/Blast/
Previous HEAD position was 5ee5e69e6 Call this Biopython 1.73
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Bio/Blast/Record.py:98:1: D102 Missing docstring in public method
Bio/Blast/NCBIXML.py:536:1: D102 Missing docstring in public method

This was from #1997, support for BLAST XML v2. So yes, I missed this during the review.

On the bright side, we're fixing these much more quickly than new code is being added - but the sooner we can enforce D102 etc automatically, the better.

@svalqui
Copy link
Contributor

svalqui commented May 22, 2019

With the above the only thing left is Bio.Entrez which is been look after by @Ikercasillas . Moving on.

@ay-amityadav
Copy link

Dear all, I have been reading Bio.Entrez and it's taking me time to understand the whole thing. I require some more time so that I can work on the concerned task, until then kindly let be assigned to me. Thanks.

@MarkusPiotrowski
Copy link
Contributor

@Ikercasillas
Maybe @mdehoon can assist/guide you; the original code is from him and he recently updated it.

@ay-amityadav
Copy link

Thanks @MarkusPiotrowski
I will surely contact @mdehoon, once I have taken a little more look at it. Also, part of the reason it's taking so much time is also that I am not able to spend enough time on it as I would have liked.

@peterjc
Copy link
Member Author

peterjc commented May 23, 2019

@Ikercasillas you've been a bit unlucky - Bio/Entrez/Parser.py was probably the hardest one to add missing docstrings too. How about you tackle the last few other files (all quite recent additions since as Markus noted, we've not been strictly enforcing this rule on recent pull requests):

$ flake8 Bio --isolated --select D102
Bio/motifs/xms.py:26:1: D102 Missing docstring in public method
Bio/motifs/xms.py:39:1: D102 Missing docstring in public method
Bio/motifs/xms.py:54:1: D102 Missing docstring in public method
Bio/motifs/xms.py:67:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:134:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:170:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:200:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:328:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:380:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:388:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:406:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:417:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:437:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:545:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:571:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:574:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:601:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:610:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:616:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:626:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:631:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:653:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:656:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:660:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:663:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:803:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:821:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:839:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:850:1: D102 Missing docstring in public method
Bio/Entrez/Parser.py:949:1: D102 Missing docstring in public method
Bio/Blast/Record.py:98:1: D102 Missing docstring in public method
Bio/Blast/NCBIXML.py:536:1: D102 Missing docstring in public method

Or, there are a few non-Entrez examples on #1961 (D101 - missing class docstrings)? Thanks!

@mdehoon
Copy link
Contributor

mdehoon commented May 23, 2019

One of the important docstring checks we currently do not enforce is missing docstrings in methods of public classes.

What is meant by public classes? I thought all classes are public.

@peterjc
Copy link
Member Author

peterjc commented May 23, 2019

I assume a class with a name starting with an underscore would be considered private - we have more of those than I would have guessed:

$ grep "class _" Bio/*.py Bio/*/*.py
Bio/File.py:class _IndexedSeqFileProxy(object):
Bio/File.py:class _IndexedSeqFileDict(_dict_base):
Bio/File.py:class _SQLiteManySeqFilesDict(_IndexedSeqFileDict):
Bio/Index.py:class _ShelveIndex(dict):
Bio/Index.py:class _InMemoryIndex(dict):
Bio/SeqRecord.py:class _RestrictedDict(dict):
Bio/Application/__init__.py:        # class _AbstractParameter.
Bio/Application/__init__.py:class _AbstractParameter(object):
Bio/Application/__init__.py:class _Option(_AbstractParameter):
Bio/Application/__init__.py:class _Switch(_AbstractParameter):
Bio/Application/__init__.py:class _Argument(_AbstractParameter):
Bio/Application/__init__.py:class _ArgumentList(_Argument):
Bio/Application/__init__.py:class _StaticArgument(_AbstractParameter):
Bio/Blast/Applications.py:class _NcbibaseblastCommandline(AbstractCommandline):
Bio/Blast/Applications.py:class _NcbiblastCommandline(_NcbibaseblastCommandline):
Bio/Blast/Applications.py:class _Ncbiblast2SeqCommandline(_NcbiblastCommandline):
Bio/Blast/Applications.py:class _NcbiblastMain2SeqCommandline(_Ncbiblast2SeqCommandline):
Bio/Blast/NCBIXML.py:class _XMLparser(ContentHandler):
Bio/Emboss/Applications.py:class _EmbossMinimalCommandLine(AbstractCommandline):
Bio/Emboss/Applications.py:class _EmbossCommandLine(_EmbossMinimalCommandLine):
Bio/GenBank/__init__.py:class _BaseGenBankConsumer(object):
Bio/GenBank/__init__.py:class _FeatureConsumer(_BaseGenBankConsumer):
Bio/GenBank/__init__.py:class _RecordConsumer(_BaseGenBankConsumer):
Bio/GenBank/Scanner.py:class _ImgtScanner(EmblScanner):
Bio/Graphics/BasicChromosome.py:class _ChromosomeComponent(Widget):
Bio/PDB/Atom.py:    # Override parent class __iter__ method
Bio/PDB/HSExposure.py:class _AbstractHSExposure(AbstractPropertyMap):
Bio/PDB/Polypeptide.py:class _PPBuilder(object):
Bio/Phylo/Consensus.py:class _BitString(str):
Bio/Phylo/TreeConstruction.py:class _Matrix(object):
Bio/SeqIO/InsdcIO.py:class _InsdcWriter(SequentialSequenceWriter):
Bio/SeqIO/SffIO.py:class _AddTellHandle(object):

Some of those are base classes from which a class for public use inherits, others are implementation details not intended for public use.

@MarkusPiotrowski
Copy link
Contributor

@svalqui Great job, and thank's to @peterjc for volunteering to smooth Entrez.Parser.py out.

@arindam31
Copy link

Just sharing my observation. I ran flake8 to catch docstring errors and I did get what I expected.

test/integration/test_configuration.py:206:1: D102 Missing docstring in public method
makefile:25: recipe for target 'codecheck' failed
make: *** [codecheck] Error 1

However, as you see, the public method is still missing the name of the method.

My version details:
3.7.9 (flake8-docstrings: 1.5.0, pydocstyle: 4.0.1, flake8-print: 3.1.4, mccabe: 0.6.1, naming: 0.8.2, pycodestyle: 2.5.0, pyflakes: 2.1.1) CPython 3.6.8 on Linux

When I use pydocstyle, I do get the expected complete result:

test/integration/test_configuration.py:206 in public method `test_configuration_reused`:
        D102: Missing docstring in public method
makefile:21: recipe for target 'doccheck' failed
make: *** [doccheck] Error 1

@peterjc
Copy link
Member Author

peterjc commented Jun 17, 2020

@arindam31 How is your query related to Biopython?

Also your flake8 and pydocstyle output quoted both identify a D102 issue on line 206 of file test/integration/test_configuration.py - they seem to be consistent.

If you use flake8, it should ignore violations as per any flake8 configuration, in our case we are currently ignoring D102 for the Tests/* files: https://github.com/biopython/biopython/blob/master/.flake8

If you use pydocstyle directly (instead of via flake8), any flake8 configuration is ignored.

@arindam31
Copy link

@arindam31 How is your query related to Biopython?

Also your flake8 and pydocstyle output quoted both identify a D102 issue on line 206 of file test/integration/test_configuration.py - they seem to be consistent.

If you use flake8, it should ignore violations as per any flake8 configuration, in our case we are currently ignoring D102 for the Tests/* files: https://github.com/biopython/biopython/blob/master/.flake8

If you use pydocstyle directly (instead of via flake8), any flake8 configuration is ignored.

Hi Peter.
I think I posted the query in the wrong group.
Just to share, I have kept pydocstyle since it shows the method name. I run my "make doccheck" using the same.
And I agree that both flake8 and pydocstyle output identify a D102 issue, but like I said, the method name is missing in former.
Perhaps, it is intended.

Thanks for your reply.

@peterjc
Copy link
Member Author

peterjc commented Jun 29, 2020

Ah, I see what you mean now. I would assume this is deliberate on the part of flake8 - their validation issues only report file and line/column number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants