search

jonocarroll / ntfy

Lightweight Wrapper to the ntfy.sh Service
Other
60 stars 3 forks source link

Add support for inline images #11

Closed jonocarroll closed 1 year ago

jonocarroll commented 1 year ago

Closes #10

jonocarroll commented 1 year ago

Ping @jfunction and @coolbutuseless for testing.

jfunction commented 1 year ago

Thanks for this - I tried a few tests and this seems to be working well. The only issue that I noticed was if you add a slash to the server, eg server="https://ntfy.sh/". In this case the message text goes through but without the attachment. You should be able to find the test script I used if you subscribe to https://ntfy.sh/jonocarroll_ntfy_testing

llrs commented 1 year ago

You might want to use conditionals in the https://github.com/jonocarroll/ntfy/pull/11/files#diff-362c106a9797decc457c98458fd736660bec3483f36d4953e2efffd2a81e4e01R80 line. I'm not sure one can have a ggplot2 objects without having the package (probably yes). (I'm thinking when/if this package is sent to CRAN to avoid errors/problems).

Nice package :smiley:

jfunction commented 1 year ago

I guess you can never be too sure, though I wonder what you'd be doing with a ggplot object without ggplot2 installed.

jonocarroll commented 1 year ago

In theory someone could try to pass in a loaded-from-disk object but the requireNamespace() is really just there to satisfy checks (not that I ran them without it) and let me leave ggplot2 in Suggests.

jonocarroll commented 1 year ago

Ah, I missed the comment by @llrs - I didn't realise that requireNamespace("pkg", quietly = TRUE) doesn't raise an error if the package "pkg" isn't available - I'll switch it to quietly = FALSE so that it's an error if ggplot2 isn't available, and should be a no-op if it's already loaded. In the rare case that it's available but not already loaded, the message is not unreasonable.

jfunction commented 1 year ago

I am not so experienced in package development, but other options for handling this (which I think @llrs is alluding to) are discussed at https://r-pkgs.org/dependencies-in-practice.html#sec-dependencies-in-suggests-r-code

jonocarroll commented 1 year ago

I've added trimming of any trailing slash on the server URL so that should always work now - I can reproduce the issue and my guess is that the additional slash is being treated as part of the topic when sending the image PUT. I needed to resort to constructing the URL including the topic because if I tried to send the image with the topic in a header I just get the 413 'JSON too large' error.

I think the requireNamespace as I have it now should be okay - it's already scoped to a block, and that block should only ever be reached if someone is already using ggplot2.

llrs commented 1 year ago

Indeed, I was hinting that ntfy has to fail gracefully when a suggested package is not available when checking in CRAN. There are several options (and I haven't checked if requireNamespace(, quietly = FALSE) is accepted). My point is that one could load an object of class ggplot2, for example with readRDS, but not have the ggplot2 package in the library/.libPath.

jonocarroll commented 1 year ago

The r-pkgs route has a hard stop() as well. I believe tests have to fail gracefully - I've had enough trouble running CRAN tests for API packages that I would just not have tests (perverse incentives and all that).