Special case imports code to run first #1538
Conversation
|
|
||
| update_namespace_imports <- function(base_path) { | ||
| NAMESPACE <- file.path(base_path, "NAMESPACE") | ||
| if (!made_by_roxygen(NAMESPACE) || !file.exists(NAMESPACE)) { |
There was a problem hiding this comment.
It seems odd to run file.exists() twice (first by made_by_roxygen(), then again directly here)
There was a problem hiding this comment.
Isn't the logic opposite? We treat a missing NAMESPACE as made by roxygen2 so we can override it. Or are you just suggesting I flip the order of the conditional?
There was a problem hiding this comment.
I'm a bit torn... from readability, it makes more sense to me like (1) check file exists (2) check if it's made by roxygen, but in implementation IINM that would mean file.exists() is always run twice, whereas the current, reverse ordering means it may only be run once.
file.exists() is of course not very expensive on most file systems so this is not v. important, sorry for the fly-by comment :)
There was a problem hiding this comment.
No problem; I had to think this through too when I wrote it, which is probably a sign it needs some refactoring, but it doesn't quite hit the threshold to be worth it.
Fixes #1144