mottosso / Qt.py

Minimal Python 2 & 3 shim around all Qt bindings - PySide, PySide2, PyQt4 and PyQt5.
MIT License
914 stars 254 forks source link

headerToModule() is destructive when CustomWidget path is a Python . path #401

Open jasonbrackman opened 6 months ago

jasonbrackman commented 6 months ago

In the following function:

def headerToModule(header):
                    """
                    Translate a header file to python module path
                    foo/bar.h => foo.bar
                    """
                    # Remove header extension
                    module = os.path.splitext(header)[0]

This will break imports if they were written in Python as the structure of the custom widget is "path.to.module"

Since the function always expects a path and a .h extension -- the above is destructive and removes the .module in my example above. Custom widgets are not possible if using Python to author the custom widget with Qt.py

Possible Fix? Patch the Qt.py: module = os.path.splitext(header)[0] if header.endswith(".h") else header

The issue here is that if its a python module you simply can't use '.h' as a module name. Its a gotcha with the above.

mottosso commented 6 months ago

Happy with this change, so long as there's a test for these two cases in https://github.com/mottosso/Qt.py/blob/master/tests.py

jasonbrackman commented 6 months ago

The following test cases are what I believe are appropriate inputs/results based on current code expectation and also addressed the bug:

    path_tests = {
        # Input: Expected

        # Valid paths; .h paths need to be updated to work with Qt.py, Python . paths should be untouched.
        "path.to.module": "path.to.module",
        "path\\to\\module.h": "path.to.module",
        "path/to/module.h": "path.to.module",
        "module.h": "module",
        "module": "module",

        # malformed; leaving the bad input QtDesigner input.
        "path.to.module.py": "path.to.module.py",   # unexpected .py
        "path\\to\\module.py": "path\\to\\module.py",  # python paths are always . paths.
        "path/to/module.py": "path/to/module.py",   # ^
        "path\\to\\module": "path\\to\\module",   #  This is malformed.  It should either be a .path for Python or have a .h ext.
        "path/to/module": "path/to/module",  # ^
        "module.py": "module.py",  # Python modules should not have have a .py extension
        }

Should Qt.py attempt to correct malformed input? If the end user entered the malformed data, I'd expect it to fail in PyQt -- so expecting that Qt.py would behave the same? I'm surprised that Qt.py has to do any .h manipulation as well -- but I don't have an example to test so have kept the 'functionality'.

mottosso commented 6 months ago

I can't quite recall how this functionality ended up in Qt.py to begin with (you can use the "Blame" button to find out) but yes, ideally it should act just like PySide2 (not PyQt5). It's important that anything written with Qt.py should be able to search-and-place with PySide2 and work just fine.

jasonbrackman commented 6 months ago

It appears that there was a PYSIDE-77 bug: https://bugreports.qt.io/browse/PYSIDE-77

Qt.py appears to have 'worked' around the issue 2 years prior to the bug being fixed? In any case the 'headerToModule' function manipulates a path that likely shouldn't be fixed at the Qt.py level. The author should really be responsible for putting in the correctly formatted path into the .ui file. However, its hard to know who is relying on the functionality now.

Let me know if there is any additional tests that should be written. Or something else that would alleviate any concerns.

jasonbrackman commented 6 months ago

Simplified the PR somewhat and the test is now an integration test instead of a unit test.

The code path first attempt to use the 'header' as authored by the user (new). If it fails to import properly, it will use the original headerToModule(...). So functionality should be what people expect and support python . paths.