Switch to autover #214
Switch to autover #214
Conversation
param/__init__.py
Outdated
| @@ -36,10 +36,13 @@ | |||
| # only two required files. | |||
| try: | |||
| from .version import Version | |||
| __version__ = Version(release=(1,5,1), fpath=__file__, | |||
| commit="$Format:%h$", reponame="param") | |||
| _version = Version(release=None, fpath=__file__, | |||
There was a problem hiding this comment.
Will autover accept (1,5,1) and do something reasonable with it, so that people who have been using the old param.version.Version won't have problems?
There was a problem hiding this comment.
I think so as autover.Version is based on the old param.Version. The issues are:
- Supporting the old behavior properly makes things more complicated.
- This old approach is not properly tested.
I'm pretty sure it will be accepted but I can't guarantee it will do the right thing! I think the original unit tests were moved to autover but I would need to check how many remain.
There was a problem hiding this comment.
I should add that we are the only people who used param.Version as far as I know (unlikely autover it wasn't a separate project and it wasn't advertised).
There was a problem hiding this comment.
One option would be to explicitly disallow these tuples making sure that projects depending on the next param release have to update to the new approach.
There was a problem hiding this comment.
We haven't advertised it recently, but it has always been available for use, the code at the top of the old Version file explained exactly how to use it in external projects, and though it's been years now, I definitely recommended it in various talks to audiences I have now forgotten.
There was a problem hiding this comment.
Shouldn't release be None by default, then?
There was a problem hiding this comment.
There was a problem hiding this comment.
The way it is specified in the line above is explicit but not strictly necessary. Explicit is good though!
There was a problem hiding this comment.
But since this is code that gets copied into every new project, I'd rather have it as short as possible here.
There was a problem hiding this comment.
Fixed in 0954adf. The version declaration shouldn't be copied verbatim between projects but I agree it is worth making this short as it is the reference example!
setup.py
Outdated
| try: | ||
| version = importlib.import_module(reponame + ".version") # Bundled | ||
| except: | ||
| version = None |
There was a problem hiding this comment.
I'll just note that the shortened version looks fine! We can return to it later but for now, it is safest to stick to what is properly tested in autover.
|
One suggestion might be to shorten the functions in Then if it all works well, we can shorten things again before the actual param release. |
|
I just unshortened the setup.py functions after your recommendation. I can't tell what all the code paths do, but I think at this stage I would just like to use it and move on. Can we merge this? |
|
I'm happy to see this merged but I would recommend that you try doing a param dev release before doing the actual release. I'm guessing @jbednar should be the one to merge or not? |
|
I can do the quick change suggested by Jim on both the autover and param repos after this is merged. Edit: Actually, why wait? I'll make the change on this PR right away. |
|
|
||
| if release is not None or 'commit_count' in kwargs: | ||
| print('WARNING: param.Version now supports PEP440 and a new tag based workflow. See param/version.py for more details') | ||
|
|
There was a problem hiding this comment.
I would like to use param.warning but that will probably cause cyclic import issues unless I inline the import? Is a print statement ok in this case and do you have any suggestions for a better message?
There was a problem hiding this comment.
This is so simple I didn't test it locally. I would wait for travis here and also on autover where I'll port this change across now. The only thing I'm wondering about is whether this print will be triggered in some of the unit tests.
|
I'm happy for it to be merged as soon as release=None is deleted, to keep the boilerplate as simple as possible. |
|
I've made the requested changes both here and to the autover repo. Params tests are green (above) and so are autover's. |
Changes:
param.__version__now complies with pep440, and is a stringparam.version.Versionuses a different versioning scheme from before; see autover for more details.