Get rid of getexistingdirectory and patch QFileDialog?

[goanpeca]

I liked the approach of py.qode better cause it did not relly on helper functions, but instead it patched QFileDialog to get the correct behavior.

We could do the same and get rid of the helper functions.

@ccordoba12 ?

[ccordoba12]

I have no problem with that. I think the current approach worked for versions older than PyQt 4.6, but they are ancient now.

PR coming?

[goanpeca]

Yep, coming soon ;-)

[Nodd]

It won't be retrocompatible, will it be qtpy 2.0 then ?

[goanpeca]

Both will work, utils will be deprecated On 2.0 we remove, which gives time for migration :)

[Nodd]

I mean if people used QFileDialog directly, the behavior may change ? But we are still the main user, I guess.

[goanpeca]

Ahh true, hmm will have to explore that :-p,

[goanpeca]

Making this change would make QFileDialog different from the Qt docs, because it will return a tuple instead of just a string.

So we either

• Do this (overload the methods) and add this in the QtPy Readme... or docs?
• Overload the QFileDialog methods to raise an error and point the user to the compat module methods...

Either way we do need to switch to 2.x

[Nodd]

I switched most of my apps to qtpy and it works great. Swithching QFileDialog methods was a bit more painful because the method names were lowercase instead of keeping the Qt case.
I think that it would be better to patch QFileDialog as it was done with other incompatibilities. As for which version to keep, I can't find the Pyside2 documentation.
Whatever solution you choose, please use the same case as Qt. It doesn"t follow pep8 but consistency is way more important than strictly following pep8. That's even written in the pep8 doc, so not following pep8 can still follow pep8 😃

[goanpeca]

@Nodd you are alive :-)

[Nodd]

@goanpeca Barely ! 😉

[cpascual]

I personally think that qtpy should not patch the imported Qt objects. The rationale is the same as I already expressed in #119 : side effects for people not even using qtpy should be avoided at all costs.

[dalthviz]

@ccordoba12 is this still relevant?

[ccordoba12]

I don't understand what this issue is about exactly, sorry.

[CAM-Gerlach]

@ccordoba12 I'm not a Qt expert like either of you, but looking at the code, seems like its about replacing the QtPy custom compatibility wrappers around the various QFileDialog static helper methods (included in this section of compat.py, that smooth over binding/platform-specific rough edges in those methods, with just monkeypatching them in the original QFileDialog object. This presumably is simpler for callers, but on the other hand means a non-backwards-compatible change in their behavior for all callers, even those not using them through QtPy or expecting the change.

As a sidenote, the current method names (all lowercase) follow neither the Qt camelCase nor the Python snake_case convention, and are much harder to read than either. For QtPy 2.0, a shift to one or the other (with the old names as deprecated aliases) might not be a bad idea.

[dalthviz]

As a sidenote, the current method names (all lowercase) follow neither the Qt camelCase nor the Python snake_case convention, and are much harder to read than either. For QtPy 2.0, a shift to one or the other (with the old names as deprecated aliases) might not be a bad idea.

I'm not totally sure about monkeypathing but providing names to this functions following the Qt camelCase convention seems like a good idea

[CAM-Gerlach]

Great, thanks. Since its a pretty straightforward change, I can help with that part, if you like and we all agree on that approach.

[dalthviz]

Sure go ahead 👍

[CAM-Gerlach]

If we're renaming and aliasing these functions anyway, we might want to consider moving the renamed functions to their own module (and keeping the old names as deprecated aliases in compat.py until the next major version), so they're more logically organized and discoverable, and aren't mixed in with the small general-purpose utility functions in compat.py.

FWIW, while I would of course defer to your and @ccordoba12 's expertise given your much deeper Qt experience, at least to my naive eye it seems the stability and compatibility impact discussed by @cpascual here and on #119 , and the rationale on mottosso/Qt.py#224 , outweighs the relatively modest inconvenience this seems to be aimed at addressing (as I understand it), especially with the renamed methods matching those in Qt itself.

[dalthviz]

Thinking about it I don't think patching is a good idea but lets check this after v2.0.0

[CAM-Gerlach]

You still want me to go ahead with the aliases, right? Since we're renaming them anyway, what are your thoughts above moving them to a more specific submodule than the catchall compat.py?

[dalthviz]

Lets leave it as it is for the moment

[CAM-Gerlach]

Okay, so not aliasing them at all, or not moving them to a different module but still aliasing them as you told me before?

[dalthviz]

What I mean to say is to not spend more time in this one, so yep not even checking the aliasing.

Seems like this is not critical at the moment and we need to carefully think if we are going to do something either patching or doing aliases to follow the camelCase convention. For me seems like this is more likely to be closed, so no patching. For the aliasing I was thinking and maybe the idea behind putting such function in neither camelCase or snake_case is to be clear that it is a helper function and is not present in Qt itself

[CAM-Gerlach]

For the aliasing I was thinking and maybe the idea behind putting such function in neither camelCase or snake_case is to be clear that it is a helper function and is not present in Qt itself

Well, if it being thin compatibility wrappers (not actually helpers) around existing Qt methods instead of the originals isn't already clear from it being imported and accessed from qtpy.compat rather than QtPy.QtWidgets, and from them being module-level functions rather than static methods of QFileDialog, it seems very doubtful that the average developer would somehow infer that from the otherwise arbitrary alllowercase of the name, at least to the extent that would justify breaking convention and making it much more difficult to read. And if we really did want to emphasize indicating they are wrappers over retaining drop-in compatibility and with the original name (in contrast to @Nodd 's statements above and the general goals of QtPy), the most logical thing to do would be to make it snake_case, so its clear its a wrapper we've implemented in our Python package rather than the original.