Options should be set in .onLoad() - if at all

[HenrikBengtsson]

Hi (again), you've got:

https://github.com/sjspielman/dragon/blob/415bb6d9a62f9b9807c86789a1948b86128d0dad/R/utils_definitions.R#L1-L9

This code is at the "top-level" of the package code. This code is evaluated when the package is installed but it is not evaluated when the package is loaded/attached. For example, despite you're trying to set options(scipen = 3), it has no effect:

> getOption("scipen")
[1] 0
> library(dragon)
> getOption("scipen")
[1] 0

If you want options and other things to be configured when your package is loaded, then you can set those in a .onLoad() function, which is called when your package is loaded.

However, having said all that, you probably don't want to set options, or a future plan, when your package is loaded. Imagine a user that sets:

options(scipen = 2)

for their scripts. Then when your package is loaded, you would silently override what the user wants. That's rarely a good idea. The same goes for the future plan. So, my suggestion is to just drop lines 1-10 in that file.

[sjspielman]

Thank you for your suggestions. Changes will be incorporated into the next release. However, certain options are necessary for package functionality and must be set with .onLoad() as I will implement.

Further, can you point me to some documentation that describes why plan() is no longer recommended? It would be much appreciated so I can properly use {future} going forward. All documentation I have seen for {future} suggests this command is necessary.

[HenrikBengtsson]

Further, can you point me to some documentation that describes why plan() is no longer recommended? It would be much appreciated so I can properly use {future} going forward. All documentation I have seen for {future} suggests this command is necessary.

The future package and plan() will keep working as before. it is specifically the multiprocess plan that is being deprecated. You can read about it on https://github.com/HenrikBengtsson/future/issues/420. I recommend to replace it with multisession.

I also advise against setting it in your package and instead give instructions to the users how to set it. The future philosophy is that it's the end-user who should control how they want to parallelize and where - not the developer (who can never know what type of resources the end-user got access to).

[HenrikBengtsson]

Regarding a package setting options and/or overriding what the user has already set;

First, before your fix (commit 7854cc76), your package did not set any options. So, I assume it has worked without those options set. By having the package set options that might be used outside of the package, you risk breaking the user's expectation. For example, you set options(scipen = 3). What if I write a package that sets options(scipen = 5). Which will be in effect depends on which package was loaded last. So, I advise against setting options unless you really really need to. Also, if you need that specific options, you can set it temporary when it's used, cf. withr::with_options(). That way won't overwrite what the user might have already set.

[sjspielman]

I also advise against setting it in your package and instead give instructions to the users how to set it. The future philosophy is that it's the end-user who should control how they want to parallelize and where - not the developer (who can never know what type of resources the end-user got access to).

Your advice is appreciated but it is not relevant to the specific extremely limited case where futures is used in this package.