search

phuhl / notify-send.py

A python-script like libnotify but with improved functionality
Other
96 stars 13 forks source link

Feature/pypi flit #5

Closed AndydeCleyre closed 4 years ago

AndydeCleyre commented 4 years ago

I'd meant to keep this small, just enough to get it up on PyPI, but couldn't help making it more use-as-module friendly along the way. If you have some time, please have a look.

FYI to install in 'editable' mode with flit: flit install -s

AndydeCleyre commented 4 years ago

If you merge this, you should be able to publish on PyPI" with:

$ python3 -m venv venv
$ . ./venv/bin/activate

$ pip install '.[dev]'
$ flit publish
AndydeCleyre commented 4 years ago

Whoops! Thank you!

In my snake casing, I'd mistakenly thought the action function was unused, confusing it with the action variable.

phuhl commented 4 years ago

I published it, but when running the pip-installed packet like notify-send.py test it gives me an additional line of output at the end:

<notify_send_py.notify_send_py.NotifySendPyCLI object at 0x....>

I am not an python-export, tbh. This does not happen when just running python3 notify_send_py/notify_send_py.py test. Do you have an idea, why this is happening and how to remove it?

AndydeCleyre commented 4 years ago

Ah, let's see. The generated script calls

sys.exit(NotifySendPyCLI())

and sys.exit does the following:

Exit the interpreter by raising SystemExit(status).

If the status is omitted or None, it defaults to zero (i.e., success).
If the status is an integer, it will be used as the system exit status.
If it is another kind of object, it will be printed and the system
exit status will be one (i.e., failure).

So it's a problem that I've made the script directly use the class constructor, which returns the class instance itself.

AFAICT this code never explicitly returns an error code anyway, so we should change the script to something that always returns either 0 or None. I will submit a new PR momentarily.

AndydeCleyre commented 4 years ago

It should be fixed by #6.

Side note: if you want to make it a bit more friendly for use-as-module, maybe notify should be updated to return n.id rather than print(n.id). Then we'd have to update NotifySendPyCLI to print that return value (only if it's an int or only if it's not None, I think).

phuhl commented 3 years ago

Hey @AndydeCleyre, I have a little trouble with flit and I thought, maybe you could take a look at it...

I had to change something within notify2 which is depricated. So I didn't do a PR upstream but just copied the notify2.py into this project. But when uploading to Pypi and installing the package, it always complains, that notify2 (or notify3 as I renamed it) was not available. I tried it as a simple import notify3 from a file called notify3.py in the notify_sind_py directory, I tried some other combinations but it would refuse to install the file.

I didn't really find a solution online either (maybe I am just looking in the wrong places...)

Do you know how this should be done? Thx

AndydeCleyre commented 3 years ago

@phuhl

  1. Can you share the relevant branch so I can take a look?
  2. Is notify3.py tracked by git? See flit docs on what gets included
  3. Is it possible to port to a non-deprecated replacement library for this purpose, maybe desktop-notify?
phuhl commented 3 years ago

@AndydeCleyre

  1. My bad, I thought, I had pushed. It is feature_paramsOnActions
  2. Yes, it is tracked by git as flit gave quite descriptive warnings about that
  3. Possible, yes, necessary, no. That would probably be a bit of reworking this script without clear benefits. I'd rather not, due to that.

Thanks for looking at it, hope I could answer all questions. Don't hesitate to ask, if something is unclear.