astro-datalab / datalab

Python client to interface with the NSF NOIRLab's Astro Data Lab Science Platform.
MIT License
22 stars 8 forks source link

Pytest to master #61

Closed iglesu closed 1 year ago

iglesu commented 1 year ago

This is a reverse merge from pytest. Meaning the pytest_to_master branched out from master and then I picked from pytest every commit not in master, except for the clients.

For the clients I performed a source code diff from the managers master client version and the datalab version, for every difference I found the original commit and added it to the commit description in this branch.

iglesu commented 1 year ago

@rnikutta @mjfitzpatrick, this branch has been successfully tested against all unit tests on the manager's side. It has also successfully executed all the command-line commands. Unless I hear otherwise, I plan to merge it next Monday, August 28th.

rnikutta commented 1 year ago

My last comment is re commit b74e19a

iglesu commented 1 year ago

Hi @iglesu , just a heads-up that dltasks.py imports __version__.py (and uses it), while the same file has an own __version__ string defined at the top. We should decide how we want to version the datalab package. I think it should be semantic versioning for the package, i.e. X.Y.Z, with version tags coined each time we build the package/release. But the __version__ string (or __version__.py) should be either unified and only reflect the edit date, or probably retired and not used at all, since git does all the "code versioning" already via commit hashes. I think we stick with __version__ out of convenience, but not out of need. We should discuss this maybe at the next DEV meeting?

@rnikutta, would you be open to submitting another PR for this? The reason behind this request is that the current PR simply encompasses a series of pending fixes. As for the version fix I've incorporated, it was primarily done to address a code error while still keeping the same use of version we had. Your suggestion regarding the version adjustment makes very much sense, but I think it merits its own PR. This way, we can distinctly mark when this modification took place.

rnikutta commented 1 year ago

Hi @iglesu I left a few inline-comments. Same thing: if you think they are better suited for separate PRs, that's OK!