Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qtpy.__version__ should be QtPy version, not Qt version #41

Merged
merged 3 commits into from Jun 15, 2016

Conversation

astrofrog
Copy link
Contributor

@astrofrog astrofrog commented Jun 15, 2016

I think that __version__ should be set to the actual QtPy version, otherwise packages won't be able to easily check for example that QtPy is recent enough to include certain patches. I think we could then make the Qt and other versions available via e.g.

qtpy.QT_VERSION
qtpy.PYSIDE_VERSION
qtpy.PYQT4_VERSION
qtpy.PYQT5_VERSION

I'd be happy to include a quick PR to include in QtPy 1.1 if others agree?

Of course, we could have qtpy.QTPY_VERSION be the QtPy version, but I think it would be counter-intuitive for __version__ to not be that.

@ccordoba12 ccordoba12 added this to the v1.1 milestone Jun 14, 2016
@ccordoba12
Copy link
Member

I agree with this because the tradition in the Python community is to use __version__ for the package version.

However, I'd like to know what @Nodd and @goanpeca think about this one.

@goanpeca
Copy link
Member

Yep I agree as well, I think it was @SylvainCorlay who did the initial packaging and used the _version.py (not complaining ;-) )

@ccordoba12
Copy link
Member

Nop, I don't think @SylvainCorlay is to be blamed here. I think this is something we inherited from spyderlib.qt.

Since @goanpeca is +1, please go ahead with this @astrofrog. I agree with the constants you proposed:

qtpy.QT_VERSION
qtpy.PYSIDE_VERSION
qtpy.PYQT4_VERSION
qtpy.PYQT5_VERSION

@Nodd
Copy link
Contributor

Nodd commented Jun 15, 2016

👍 too !
We could have another set of constants containing the version as tuples for easy comparison (like sys.versioninfo).

@astrofrog
Copy link
Contributor Author

astrofrog commented Jun 15, 2016

Code attached - note that for now, I actually went with:

qtpy.QT_VERSION
qtpy.PYSIDE_VERSION
qtpy.PYQT_VERSION

I could easily split PYQT_VERSION into PYQT4_VERSION and PYQT5_VERSION, but do we want that? (if so, do we want QT4_VERSION and QT5_VERSION? I think not, since at the end of the day it defies the point if part of the version is already in the variable name?)

@astrofrog
Copy link
Contributor Author

I'm not sure what convention you want to use for the __version__ number, but I went with what I would use for other projects - let me know if you want to change that.

@Nodd
Copy link
Contributor

Nodd commented Jun 15, 2016

What did you do to transform the initial issue into a PR ? I didn't know that it was possible.

@astrofrog
Copy link
Contributor Author

✨ Magic ✨

Actually I just used the hub command 😄

@astrofrog
Copy link
Contributor Author

Once this is merged, then from my point of view we're good for a 1.1 release from the point of view of glue (all our tests pass).

@Nodd
Copy link
Contributor

Nodd commented Jun 15, 2016

Only one of PYSIDE_VERSION and PYQT_VERSION are defined at a time. I think it's better if the values are set to None by default rather than not being defined. Getting None is more understandable than getting an AttributeError.

@astrofrog
Copy link
Contributor Author

@Nodd - oops, of course, sorry about that

@@ -52,6 +52,9 @@

import os

# Version of QtPy
__version__ = "1.1.dev0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__version__ is defined here:

https://github.com/spyder-ide/qtpy/blob/master/qtpy/_version.py#L2

Please use that here instead :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@ccordoba12
Copy link
Member

Just one minor comment, after that we can merge and release 1.1

@astrofrog
Copy link
Contributor Author

@ccordoba12 - this should be ready to go

@Nodd Nodd merged commit 69f16d3 into spyder-ide:master Jun 15, 2016
@Nodd
Copy link
Contributor

Nodd commented Jun 15, 2016

@ccordoba12 I merged, you can do the release !

@ccordoba12
Copy link
Member

I think I need to do first a PR in Spyder because of this change. I'll let you know about it :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants