search

sot / chandra_time

Convert between various time formats relevant to Chandra.
https://sot.github.io/Chandra.Time
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Modernize for test compatibility #24

Closed jeanconn closed 7 years ago

jeanconn commented 7 years ago

Move Time.py to subdir to fix paths for testr

jeanconn commented 7 years ago

This still needs testing in a ska, but does this make sense as a fix for an incorrect path guess from testr? I think testr.test expects to go up 2 directories from Chandra/Time to run the tests, but Chandra/Time does not exist as a directory in this package.

taldcroft commented 7 years ago

If you are going down this path (YES, recommended!) then might as well bring it up to the modern standard: http://cxc.harvard.edu/mta/ASPECT/tool_doc/testr/#python-testing-helpers.

jeanconn commented 7 years ago

Though regarding moving Time.py to a directory, it looks like quite a few of the namespace package modules are set up this way (Chandra/Time.py, Chandra/Maneuver.py, Ska/File.py). Is there a reasonable way to help testr find either style without getting crazy about it?

taldcroft commented 7 years ago

That package organization (e.g. Chandra/Time.py) prevents distributing inline tests, so I would rather just bite the bullet and do things right. Basically that is just the wrong way to do things and a remnant of early lack of knowledge.

Note that in testr a package without bundled inline tests should use a shell test script anyway and call py.test (or whatever) directly.

jeanconn commented 7 years ago

Ah. I was confused about the definition of bundled inline tests, because Chandra.Time already had a test() method to call its own test. But I wasn't sure about the py.test magic to get it to work to report failures.

taldcroft commented 7 years ago

Pytest is really not so magical. When run from the command line it just gives a shell exit value of 1 if tests fail, and that signals testr that there was a failure. When run as a main script it returns the number of failures.

jeanconn commented 7 years ago

I put in code to use a version.py but then removed it, because I'm not sure how to require that the code builds _axTime3.so before trying to import Chandra.Time (and that's a apparently problem if I try to import Chandra.Time.version or the like if I put version.py in Chandra/Time/).

taldcroft commented 7 years ago

No problem on the version. Astropy has some tricky machinery to handle this problem (injecting globals in the Python interpreter built-ins), but we don't need to do this.

jeanconn commented 7 years ago

Does MANIFEST.in need an update to match this move?

(I email replied with something like this earlier this evening but that looks lost).

jeanconn commented 7 years ago

Also, does the package and install process need to change? If I do python setup.py sdist and try to install the package in my dev ska from skare I end up with

    ValueError: 'Chandra/Time/_axTime3.pyx' doesn't match any files
jeanconn commented 7 years ago

It looks like the MANIFEST.in change seems to fix my missing Chandra/Time/_axTime3.pyx issue, so maybe that's a two-for.

taldcroft commented 7 years ago

It wouldn't hurt, but I think that the spirit of PEP8 is about making external dependencies visible and well-organized. This is an internal package module and is not really a dependency.

taldcroft commented 7 years ago

Post-facto :+1: for merge.