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

QtPyConvert modifications #285

Merged
merged 24 commits into from
Apr 24, 2018

Conversation

Ahuge
Copy link
Collaborator

@Ahuge Ahuge commented Apr 17, 2018

Hey @mottosso and team.

This is a patchset that we have applied for QtPyConvert and is something that we require for introspecting against Qt.py

Could we take some time in the near future to talk through these changes and merge them in if we are all ok with them?

Qt.py Outdated
@@ -1726,3 +1819,4 @@ def _install():
# CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
# TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
# SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

Choose a reason for hiding this comment

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

blank line at end of file

@Ahuge
Copy link
Collaborator Author

Ahuge commented Apr 17, 2018

Travis has some issues.

HOLD on this until I fix those.

Alex Hughes added 3 commits April 17, 2018 13:21
Also updating the _setup code to attempt importing the top level extras too if the module level ones failed.
Pulling getCppPointer out to the module scope for introspection.
…it into this branch yet. Woops. This appears to fix those issues in my local nosetester.
Qt.py Outdated
dst_value = _part
except AttributeError:
# If the member we want to store in the namespace does not exist,
# there is no need to continue. This can happen if a request was

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

Qt.py Outdated
_part = getattr(_part, member)
dst_value = _part
except AttributeError:
# If the member we want to store in the namespace does not exist,

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

@Ahuge
Copy link
Collaborator Author

Ahuge commented Apr 17, 2018

Canceling this for now. I want to make some more changes

@Ahuge Ahuge closed this Apr 17, 2018
@Ahuge
Copy link
Collaborator Author

Ahuge commented Apr 17, 2018

Was having issues testing locally but think I got it now.

@Ahuge Ahuge reopened this Apr 17, 2018
Copy link
Owner

@mottosso mottosso left a comment

Choose a reason for hiding this comment

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

Great job! Especially like the updated misplaced_members, smart.

Qt.py Outdated

"""Default binding ordering.

Brought into globals so I can introspect against it
Copy link
Owner

Choose a reason for hiding this comment

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

This seems better suited for a commit message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed!

Qt.py Outdated
return the newly created instance of the user interface.

"""
# Not sure where this code came from.
Copy link
Owner

Choose a reason for hiding this comment

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

If you select a line and hit "Blame" in the GitHub GUI, it'll take you to where that line was introduced, along with the commit message for it.

It was deleted recently, but I can't locate the PR for it. :S Nevertheless, you can remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed!

@@ -881,7 +1180,13 @@ def _setup(module, extras):
submodule = _import_sub_module(
module, name)
except ImportError:
continue
try:
Copy link
Owner

Choose a reason for hiding this comment

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

What's happening here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allows support for keeping sip/shiboken around in Qt.
I am using this for the changes in the getCppPointer unified function above.
If you look at L1389 for example, we are adding "shiboken" and "sip" to the extras that get passed into this.
This part allows us to bind it to Qt._sip and Qt._shiboken.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I see. I wonder whether this is better suited to be hosted inside your project. I'd expect access to Qt._sip to break with running under PySide, and vice versa?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Under PySide Qt._sip doesn't exist. It just wouldn't be an attribute. The extras list is specific to each binding's setup function.

This was just support for stuff that used those binding specific modules to defer the actual function call to the unified _wrapinstance for example

Copy link
Owner

@mottosso mottosso Apr 18, 2018

Choose a reason for hiding this comment

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

Ok, I suppose so long as it has the _-prefix then it shouldn't cause any trouble.

The reason I'm weary is because the underscore == private concept isn't obvious to everyone; I sometimes get questions and feature requests for those, where they've already built their tool around it.

Then I have to explain that "oh, yes it is useful and you can technically use it, but you shouldn't". They won't care, until we remove it at which point they come back "Hey, it's broken, you suck!" :)

I forget, if it has a double-underscore, can you still access it externally, e.g. Qt.__sip?

Copy link
Collaborator

@fredrikaverpil fredrikaverpil Apr 18, 2018

Choose a reason for hiding this comment

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

I forget, if it has a double-underscore, can you still access it externally, e.g. Qt.__sip?

You should be able to access it, but the linters will complain. And so would hound, I presume?

printf "_x = True\n__x = True" > privtest.py
python3.6 -c "import privtest; print(privtest._x); print(privtest.__x)"
True
True

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically linters are going to get mad at QtPyConvert using any private underscore things from Qt.py

So I think this is an acceptable solution for QtPyConvert if you are ok with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Hound is great though, as it keeps Qt.py tidy. Unfortunately, it doesn't seem like you can tell hound to ignore certain linting errors: houndci/hound#1184

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can make it a double underscore if we'd like, however it might make more sense to think about
"an api" to develop on top of. @mottosso brought this up in the gitter chat briefly.

Would you be ok if we kept it private for now and once we decide about some sort of Qt.py api we can discuss moving it then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

All fine by me. I honestly haven't had time to go through this PR properly and will leave it up to you guys if you're okay with that.

Qt.py Outdated
preferred_order = list(
b for b in QT_PREFERRED_BINDING.split(os.pathsep) if b
)

order = preferred_order or default_order
order = preferred_order or _QT_PREFERRED_BINDING_DEFAULT_ORDER
Copy link
Owner

Choose a reason for hiding this comment

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

I think if you want access to these we should probably handle the above parsing from environment variable to list at module-level too, so you get the same order the user has specified. Could possibly move the preferred_order parsing to the top of the module.

Otherwise, I expect what may happen is users complaining that they aren't able to control the resolution order, even though "it works on in Maya" etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm just using it as a list of bindings supported by Qt.py

I don't need the order or, as far as I can think, the subset that they want to support.
It's used to programmatically decide if a module is supported by Qt.py and if I am to rely on Qt.py for conversion or to see if there are custom mappings that the user provided.

Copy link
Owner

Choose a reason for hiding this comment

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

The supported bindings is fixed and won't change. Can you redefine this list internally in QtPyConvert? It'd mean one less module-level constant to worry about in Qt.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, I'll just ingest it into QtPyConvert

@fredrikaverpil
Copy link
Collaborator

Just for anyone else who might want to use this version of Qt.py against running code, it can be pip-installed easily:

pip install git+https://github.com/digitaldomain/Qt.py.git@QtPyConvert_modifications

Alex Hughes added 2 commits April 18, 2018 08:52
Qt.py Outdated
try:
try:
# Before merge of PySide and shiboken
import shiboken
except ImportError:
# After merge of PySide and shiboken, May 2017
from PySide import shiboken
extras.append("shiboken")
except ImportError:
shiboken = None

Choose a reason for hiding this comment

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

local variable 'shiboken' is assigned to but never used

Qt.py Outdated
try:
try:
# Before merge of PySide and shiboken
import shiboken2
except ImportError:
# After merge of PySide and shiboken, May 2017
from PySide2 import shiboken2
extras.append("shiboken2")
except ImportError:
shiboken2 = None

Choose a reason for hiding this comment

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

local variable 'shiboken2' is assigned to but never used

@Ahuge
Copy link
Collaborator Author

Ahuge commented Apr 23, 2018

Hey, just checking in, are you waiting on me for something still? I just want to make sure I am not holding you back.

@mottosso
Copy link
Owner

Sorry, it had slipped my mind. It looks allright to me, let's flip the switch!

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.

4 participants