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

Depend on pyperclip on all platforms #155

Merged
merged 4 commits into from
Jan 19, 2025
Merged

Depend on pyperclip on all platforms #155

merged 4 commits into from
Jan 19, 2025

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Jan 17, 2025

This should help with #153

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.69%. Comparing base (80039f8) to head (bba574e).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/em_keyboard/__init__.py 78.57% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   98.49%   95.69%   -2.80%     
==========================================
  Files           2        2              
  Lines         199      209      +10     
==========================================
+ Hits          196      200       +4     
- Misses          3        9       +6     
Flag Coverage Δ
macos-latest 92.82% <76.47%> (-0.65%) ⬇️
ubuntu-latest 93.30% <79.41%> (-0.17%) ⬇️
windows-latest 92.82% <76.47%> (-1.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hugovk
Copy link
Owner

hugovk commented Jan 17, 2025

Hmm, looks like pyperclip isn't happy if xclip or xselect are not already installed:

tests/test_em.py:31: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/py/lib/pypy3.10/site-packages/em_keyboard/__init__.py:1[68](https://github.com/hugovk/em-keyboard/actions/runs/12828138604/job/35797313021?pr=155#step:5:69): in cli
    copier.copy(results)
.tox/py/lib/pypy3.10/site-packages/pyperclip/__init__.py:622: in lazy_load_stub_copy
    return copy(text)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <pyperclip.init_no_clipboard.<locals>.ClipboardUnavailable object at 0x00007f2de[70](https://github.com/hugovk/em-keyboard/actions/runs/12828138604/job/35797313021?pr=155#step:5:71)b6e20>
args = ('⭐',), kwargs = {}
additionalInfo = '\nOn Linux, you can run `sudo apt-get install xclip` or `sudo apt-get install xselect` to install a copy/paste mechanism.'

    def __call__(self, *args, **kwargs):
        additionalInfo = ''
        if sys.platform == 'linux':
            additionalInfo = '\nOn Linux, you can run `sudo apt-get install xclip` or `sudo apt-get install xselect` to install a copy/paste mechanism.'
>       raise PyperclipException('Pyperclip could not find a copy/paste mechanism for your system. For more information, please visit https://pyperclip.readthedocs.io/en/latest/index.html#not-implemented-error' + additionalInfo)
E       pyperclip.PyperclipException: Pyperclip could not find a copy/paste mechanism for your system. For more information, please visit https://pyperclip.readthedocs.io/en/latest/index.html#not-implemented-error
E       On Linux, you can run `sudo apt-get install xclip` or `sudo apt-get install xselect` to install a copy/paste mechanism.

@hukkin
Copy link
Contributor Author

hukkin commented Jan 18, 2025

I had difficulties having the copier module and tox work together, so mocked the copier away on Linux. This really isn't optimal. Any thoughts or ideas?

Things I tried included:

  • set pytest -s in tox
  • set clipboard tools that pyperclip calls in tox's allowlist_externals

@hugovk hugovk added the changelog: Added For new features label Jan 18, 2025
@hugovk
Copy link
Owner

hugovk commented Jan 18, 2025

I think this is okay, we don't need to (or want to) test the functionality of the copier itself.

Instead, we want to test the functionality of em-keyboard when the copier is present or absent.

I take it you've tested the em CLI actually works okay with the copier on Linux?

What happens when:

  • copier is installed, but the system dependency is not installed?
  • copier is not installed, but the system dependency is installed?
  • both are installed?
  • neither are installed?

@hukkin
Copy link
Contributor Author

hukkin commented Jan 19, 2025

I take it you've tested the em CLI actually works okay with the copier on Linux?

Yes, it works with xclip!

What happens when:

  • copier is installed, but the system dependency is not installed?

Current state is that a PyperclipException exception is raised by pyperclip. It is not caught by em-keyboard. The exception message includes: "On Linux, you can run sudo apt-get install xclip or sudo apt-get install xselect to install a copy/paste mechanism."

  • copier is not installed, but the system dependency is

em-keyboard behaves as if --no-copy was used.

  • both are

Copying to clipboard works as expected.

  • neither are installed?

em-keyboard behaves as if --no-copy was used.

@hukkin
Copy link
Contributor Author

hukkin commented Jan 19, 2025

I think we may want to either:

  • catch the PyperclipException if system dependency is missing and make behavior the same as if pyperclip is missing (i.e. an implicit --no-copy)
  • Always hard error if pyperclip or system dependency is missing to guarantee the program always works as shown in the docs. Make sure that it still works if --no-copy is explicitly given.

@hugovk
Copy link
Owner

hugovk commented Jan 19, 2025

I think we may want to either:

  • catch the PyperclipException if system dependency is missing and make behavior the same as if pyperclip is missing (i.e. an implicit --no-copy)

Yes, I think this would be better: right now with this PR, a normal install will also install pyperclip, but you may well not have the system dependency, and it's not very friendly to just error out.

@hukkin
Copy link
Contributor Author

hukkin commented Jan 19, 2025

Great. With the last commit this should now never error. If any copy-to-clipboard related dependency is missing, it should work as if --no-copy was used.

@hugovk
Copy link
Owner

hugovk commented Jan 19, 2025

Thank you!

@hugovk hugovk merged commit ff2bdef into hugovk:main Jan 19, 2025
29 of 31 checks passed
@hukkin hukkin deleted the clipboard branch January 19, 2025 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants