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

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

Open
jasonbrackman opened this issue Mar 12, 2024 · 5 comments

Comments

@jasonbrackman
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

jasonbrackman commented Mar 13, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants