search

bnosac / cronR

A simple R package for managing your cron jobs.
Other
288 stars 38 forks source link

Use normalizePath #64

Closed llrs closed 10 months ago

llrs commented 10 months ago

This PR is for #63, to ensure that the path is absolute and not relative.

jwijffels commented 10 months ago

I´m not sure if adding normalizepath inside the function is a good idea. What if normalizepath fails for the reasons indicated in the help of normalizepath? Putting a warning in case the path is not a full path makes more sense to me. Unless you can guarantee that normalizepath will work in all cases where this change drops some kind of freedom from the user.

llrs commented 10 months ago

I cannot guarantee what R itself doesn't, so yes this drops some kind of freedom.

The command is currently printed before being approved, if the user doesn't pay attention now it can lead to an missing script or an incorrect path, the same as with normalizePath. But it can use mustWork = TRUE and the user will be happy to know that the path provided doesn't have the script or it doesn't work. A warning can be helpful too. Feel free to choose what you consider as best.

jwijffels commented 10 months ago

I'm going to close this. This seems to drop some freedom which might open edge cases which I'm not aware of.

I'm open however for a pull requests which prints a warning message if the normalizePath gives something different than the provided rscript / workdir but I'd prefer to leave it up to the user to specifiy the path correctly - I don't want to give the impression that cronR manages handling paths.