sot / kadi

Chandra commands and events
https://sot.github.io/kadi
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Changes to use scm_setuptools #150

Closed javierggt closed 4 years ago

javierggt commented 4 years ago

Since I do not know how this affects backwards compatibility, I tried to leave the interface mostly unchanged. I think each package might have to be fixed to cleanup its interface.

In the case of kadi, the version is specified in various ways:

Right now all come from the same source.

I expect the changes in eng_archive to be similar

taldcroft commented 4 years ago

I suspect that much or all of the complexity in the current versioning output is really not used anywhere. E.g. https://github.com/search?q=org%3Asot+git_version&type=Code shows 9 places defining git_version but no configured code actually using it. The problem this was solving is being able to relate the version of imported code back to a specific commit or tag of the package. With setuptools_scm we can now do this reliably, without hacks like writing a GIT_VERSION file.

Which is a long way of saying that I propose just chopping the version.py file entirely and get rid of this historical cruft. So long as all testing passes, then we're good to go. Keep this PR open for a little while for reference, but basically I think we can say we are making a back-incompatible API change and if code needs fixing then we do it (fix and forward).

taldcroft commented 4 years ago

Have not tested but this all looks about right to me.

jeanconn commented 4 years ago

A nit, but probably also need an update in docs/conf.py .

jeanconn commented 4 years ago

And just searching for other uses of kadi.version it looks like there's one in find_attitude, so that probably needs a new issue

https://github.com/sot/find_attitude/blob/cefd1923ad2ddde2496f98943f6fb1e2b504866a/find_attitude/web/views.py#L5

if I'm understanding correctly that kadi.version will just not exist.

javierggt commented 4 years ago

Good catch. I will look for similar issues all over

javierggt commented 4 years ago

Looking for similar cases, I found a few, including:

jeanconn commented 4 years ago

Yes, when you/we do starcheck we'll need to clean up some of its internal use of starcheck.version. And yes, that string in starcheck.pl is used asInline::Python block (though I think it is a separate starcheck issue to pull out more of the Python into files that can be appropriately flake8 checked and such instead of the string blocks).

jeanconn commented 4 years ago

I think this one is just superseded by #143