manoharan-lab / holopy

Hologram processing and light scattering in python
GNU General Public License v3.0
131 stars 50 forks source link

Develop #347

Closed barkls closed 4 years ago

barkls commented 4 years ago

Supersedes #334 and also cleans up test import structure

pep8speaks commented 4 years ago

Hello @barkls! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 311:21: E251 unexpected spaces around keyword / parameter equals Line 311:23: E251 unexpected spaces around keyword / parameter equals Line 311:34: E251 unexpected spaces around keyword / parameter equals Line 311:36: E251 unexpected spaces around keyword / parameter equals

Line 31:80: E501 line too long (80 > 79 characters)

barkls commented 4 years ago

removing the test init files will prevent sphinx apidoc from building files for the test directories since they won't be treated as submodules any more. Tests pass with them removed, but there might be unintended consequences, such as files not getting carried around by conda. I don't think anyone is trying to run tests of a conda distribution though, and inference tests never had init.py to begin with.

barkls commented 4 years ago

I don't think we want to have the tests in the HoloPy namespace. Is there a use case for importing test code (including common) when you can't just run the file directly? I see imports as being useful when you want to be able to run the code but not edit it, and I'm not sure when this would ever apply to tests.

That being said, I don't feel strongly about it and we can certainly put the init files back in, which could also have the side-effect of resolving #342. Currently things are inconsistent with some test directories having an __init__.py and others not, so we could apply a standard either way.

briandleahy commented 4 years ago

Sounds good. We can always add the inits back again if we need to later.

Merging this in now.