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

Update clipboard Qt-bindings for flexiblity and Python3 compatibility #17723

Merged
merged 15 commits into from
Nov 24, 2017
Merged

Update clipboard Qt-bindings for flexiblity and Python3 compatibility #17723

merged 15 commits into from
Nov 24, 2017

Conversation

dvincentwest
Copy link
Contributor

@dvincentwest dvincentwest commented Sep 29, 2017

This should prevent any conflicts with other qt-bindings packages when embedding pandas in a Qt-based gui and should also provide compatibility with Python3 since PyQt4 may not be available for the latest releases.

…r the clipboard. This should prevent any conflicts with other qt-bindings packages when embedding pandas in a Qt-based gui and should also provide compatibility with Python3 since PyQt4 may not be available for the latest releases.
@codecov
Copy link

codecov bot commented Sep 29, 2017

Codecov Report

Merging #17723 into master will decrease coverage by 0.05%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17723      +/-   ##
==========================================
- Coverage   91.35%   91.29%   -0.06%     
==========================================
  Files         163      164       +1     
  Lines       49695    49747      +52     
==========================================
+ Hits        45401    45419      +18     
- Misses       4294     4328      +34
Flag Coverage Δ
#multiple 89.09% <60%> (-0.05%) ⬇️
#single 40.48% <60%> (+0.75%) ⬆️
Impacted Files Coverage Δ
pandas/io/clipboard/__init__.py 56.86% <40%> (-19.33%) ⬇️
pandas/io/clipboard/clipboards.py 32.18% <80%> (+8.13%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.44% <0%> (-1.82%) ⬇️
pandas/tseries/offsets.py 96.7% <0%> (-0.31%) ⬇️
pandas/core/resample.py 96.16% <0%> (-0.19%) ⬇️
pandas/core/dtypes/missing.py 90.62% <0%> (-0.12%) ⬇️
pandas/core/indexes/datetimes.py 95.39% <0%> (-0.11%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6eac0b...53ee4bc. Read the comment docs.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

You'll also want to update doc/source/install.rst under Optional Dependencies

# Check if PyQt4 is installed
import PyQt4 # noqa
# Check if qtpy is installed
import qtpy # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this fall back to PyQt4 if qtpy isn't available? I'm Ok with making qtpy the default, but we don't want to break a user's install that just has PyQt4.

@sinhrks sinhrks added Compat pandas objects compatability with Numpy or Python functions IO Data IO issues that don't fit into a more specific label labels Sep 29, 2017
@sinhrks
Copy link
Member

sinhrks commented Sep 29, 2017

Should update io.rst too.

@dvincentwest
Copy link
Contributor Author

I will make all these updates within a couple of days


def copy_qt(text):
cb = app.clipboard()
cb.setText(text)

def paste_qt():
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove this?

@dvincentwest
Copy link
Contributor Author

@asweigart just pushed some commits to pyperclip, more than I was tackling here, but I reverted the code to his latest commits regarding this issue. Behavior now is to import qtpy, then PyQt5 if that fails, then PyQt4 if that fails. Should not break for anyone who was using this functionality previously. Documentation was also updated as requested. I am waiting for CI checks to pass, and will check back to make sure that they do

@dvincentwest
Copy link
Contributor Author

dvincentwest commented Oct 30, 2017 via email

@dvincentwest
Copy link
Contributor Author

it appears I violated style standards with 2 greater than 80 character lines. I should have read the contribution documentation more carefully. Apologies, I will review that, fix the lines, and update.

@jreback
Copy link
Contributor

jreback commented Oct 31, 2017

ideally would like to enhance 1 or more of our CI builds to use qtpy. Some of them are already testing via xsel/python-gtk2. they are a bit flaky though. can you add in pyqt to a couple of builds (that don't otherwise add the x selections). the clipboard tests themselves exercise this.

@dvincentwest
Copy link
Contributor Author

@jreback, I've never done that, but in the spirit of learning I will try, but will be careful before making any changes. On a second note, the last round of tests failed because ModuleNotFoundError was not recognized as a valid exception. Why is that? I changed the exception to be ImportError instead, which is valid and should work, but I was wondering why the error with ModuleNotFoundError.

@jreback
Copy link
Contributor

jreback commented Oct 31, 2017

ModuleNotFoundError
is python3 only. ImportError will be raised for py2

@dvincentwest dvincentwest changed the title 2 code changes and docstring changes to use qtpy in place of PyQt4 fo… Update clipboard Qt-bindings for flexiblity and Python3 compatibility Oct 31, 2017
@ccordoba12
Copy link

I'm coming here from spyder-ide/spyder#5795. This problem is also generating odd errors for us in Spyder, so I want to chime in to see when this is going to be merged and if there's something I can do to help with.

@jreback and @TomAugspurger, I think Pandas should depend directly on qtpy. qtpy is a tiny dependency and it would make the code added by @dvincentwest (which looks right to me) very, very simple, instead of a lot of try/excepts (which are already done in qtpy itself).

Besides, it would be easier to handle future renames or changes in Qt by depending on qtpy because qtpy will take care of them.

@TomAugspurger
Copy link
Contributor

Thanks.

I don't think adding qtpy as a dependency is worthwhile, since read_clipiboard isn't a core part of pandas.

@dvincentwest could you add a release note? I think in doc/source/whatsnew/v0.22.0.txt. Other than that, the changes here look good.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

you need to add qtpy to the ci/requirements-3.6_BUILD_TEST.sh

@@ -251,7 +251,8 @@ Optional Dependencies
* `Jinja2 <http://jinja.pocoo.org/>`__: Template engine for conditional HTML formatting.
* `s3fs <http://s3fs.readthedocs.io/>`__: necessary for Amazon S3 access (s3fs >= 0.0.7).
* `blosc <https://pypi.python.org/pypi/blosc>`__: for msgpack compression using ``blosc``
* One of `PyQt4
Copy link
Contributor

Choose a reason for hiding this comment

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

you can list qtpy here as well

@@ -127,6 +127,7 @@ I/O
- Bug in :func:`read_msgpack` with a non existent file is passed in Python 2 (:issue:`15296`)
- Bug in :func:`read_csv` where a ``MultiIndex`` with duplicate columns was not being mangled appropriately (:issue:`18062`)
- Bug in :func:`read_sas` where a file with 0 variables gave an ``AttributeError`` incorrectly. Now it gives an ``EmptyDataError`` (:issue:`18184`)
- :func:`pandas.read_clipboard` updated to use qtpy, falling back to PyQt5 and then PyQt4, adding compatibility with Python3 and multiple python-qt bindings (:issue:`17722`)
Copy link
Contributor

Choose a reason for hiding this comment

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

move to other enhancements

@jreback
Copy link
Contributor

jreback commented Nov 21, 2017

does installing qtpy actually installing a pyqt binding? e.g the qt package?

@dvincentwest
Copy link
Contributor Author

No, qtpy is only a compatibility later so that you can write bindings agnostic applications. It still requires either Pyqt(4 or 5) or Pyside(1 or 2)

@jreback
Copy link
Contributor

jreback commented Nov 22, 2017

No, qtpy is only a compatibility later so that you can write bindings agnostic applications. It still requires either Pyqt(4 or 5) or Pyside(1 or 2)

ok can you make that clear in the install.rst instructions. also do add to one of the ci builds.

@dvincentwest
Copy link
Contributor Author

yes, I will try to get this done tomorrow...

@dvincentwest
Copy link
Contributor Author

@jreback @TomAugspurger , the Python 2.7 appveyor build appears to have gone over the 1 hour time-limit. I am not sure why, and I don't really know what to do about that next.

I did add pyqt (conda label for PyQt5) and qtpy to the ci/requirements-3.6_BUILD_TEST.sh script but I have never dealt with CI before so I think you should have a quick look to make sure I did what you were looking for.

@jreback jreback added this to the 0.22.0 milestone Nov 24, 2017
@jreback jreback merged commit 412988e into pandas-dev:master Nov 24, 2017
@jreback
Copy link
Contributor

jreback commented Nov 24, 2017

thanks @dvincentwest

@dvincentwest
Copy link
Contributor Author

And my thanks to you guys for maintaining the project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat pandas objects compatability with Numpy or Python functions IO Data IO issues that don't fit into a more specific label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Qtbindings clipboard conflict
5 participants