ionelmc / cookiecutter-pylibrary

Enhanced cookiecutter template for Python libraries.
BSD 2-Clause "Simplified" License
1.25k stars 207 forks source link

config env:spell according to doctest #160

Closed joaomcteixeira closed 5 years ago

joaomcteixeira commented 5 years ago

As far my understanding goes, selection doctest during the setup introduces .. testsetup:: in the reference/sampleproject.rst file.

This creates incompatibilities with the env:spell in tox because the option skip_install is activated:

WARNING: autodoc: failed to import module 'sampleproject'; the following exception was raised:
No module named 'sampleproject'

In doctest is selected, usedevelop should be used instead of skip_install.

I have added this option in the tox.ini templates. This issue directly relates with #159

Let me know your opinion and if more additions/changes in the code are required to complete the implementation.

ionelmc commented 5 years ago

Does that warning influence the exit code?

joaomcteixeira commented 5 years ago

No. It just sends a warning. (i dont like red lines :P)

WARNING: autodoc: failed to import module 'sampleproject'; the following exception was raised:
No module named 'sampleproject'
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] usage
Spelling checker messages written to /home/joao/GitHub/python-bioplottemplates/dist/docs/output.txt
build succeeded, 1 warning.
__________________________________________________________________________________________ summary ___________________________________________________________________________________________
  spell: commands succeeded
  congratulations :)

A second question: are doctests performed under this configuration? Why using and implementing .. testsetup:: if at the end it won't work?

dHannasch commented 5 years ago

A second question: are doctests performed under this configuration?

doctests are already performed by testenv:docs, so I don't think we want them to also be performed by testenv:spell. https://github.com/ionelmc/cookiecutter-pylibrary/blob/master/%7B%7Bcookiecutter.repo_name%7D%7D/ci/templates/tox.ini#L134

As far as I can tell, testenv:spell indeed does not perform doctests. The only reason it's trying to import the module is for autodoc. Honestly...personal opinion, it seems like it might make more sense to just disable autodoc for testenv:spell, since the only point in having it is to spell-check the autodocs. And I don't think there's any point in spell-checking the autodocs.

Mind you, I don't know off-hand any way to do that that wouldn't be ugly. We could put a try-except in https://github.com/ionelmc/cookiecutter-pylibrary/blob/master/%7B%7Bcookiecutter.repo_name%7D%7D/docs/reference/%7B%7Bcookiecutter.package_name%7D%7D.rst and just have it silently fail to import if the module isn't installed, but that seems sketchy.

Why do we even have testenv:spell as a separate testenv from testenv:docs? Should spelling just be an option for testenv:docs like doctests itself? Ah, because spelling is annoying in how it constantly makes builds fail for unknown words: https://github.com/ionelmc/cookiecutter-pylibrary/pull/159#issuecomment-540951174

We could explicitly pip install . in https://github.com/ionelmc/cookiecutter-pylibrary/blob/master/%7B%7Bcookiecutter.repo_name%7D%7D/ci/templates/.travis.yml and then tox --sitepackages. Of course, that wouldn't affect running locally (when you usually don't want to use --sitepackages).

joaomcteixeira commented 5 years ago

Hi @dHannasch ,

Well, making builds fail for unknown words I think is a nice feature, at least one I want in my projects, and new words can just be added to the 'spelling' file.

I guess here we enter the details of preference rather then good practices maybe? Just a pure technical question what is wrong on the following?

[testenv:spell]                                                                 
setenv =                                                                        
    SPELLCHECK=1                                                                
commands =                                                                      
    sphinx-build -b spelling docs dist/docs                                     
usedevelop = true                                                               
deps =                                                                          
    -r{toxinidir}/docs/requirements.txt                                         
    sphinxcontrib-spelling                                                      
    pyenchant 

Does it causes problems of any sort using userdevelop = true to avoid all non-pretty turn-arounds as you mentioned? I just want to understand the technical reasons for you rejection of this option.

Best,

ionelmc commented 5 years ago

Just to explain: the spell env is a separate one because it requires system libs and it's a fussy ordeal - not everyone needs or have patience to whitelist all sorts of words. Also pyenchant is unmaintained 😒

Maybe we should remove it. Most editors already have a spell checker.

joaomcteixeira commented 5 years ago

After all this discussion I agree with you, specially because it depends on system libs.