Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop generator attributes #173

Closed
wants to merge 1 commit into from
Closed

Conversation

hadley
Copy link
Collaborator

@hadley hadley commented Aug 5, 2024

They don't appear to be used for anything, and interfere with code navigation.

They don't appear to be used for anything, and interfere with code navigation.
@daroczig
Copy link
Owner

daroczig commented Aug 5, 2024

The purpose of the generator attribute is to keep track how the formatter/layout/appender fn was defined, and be able to report on that e.g. when the user checks what is being used:

library(logger)
log_appender()
#> appender_console

log_appender(appender_file(file = "/tmp/foobar"))
log_appender()
#> appender_file(file = "/tmp/foobar")

If we don't keep track of this, then the user will see something like:

function (lines) 
{
    if (is.finite(max_lines) | is.finite(max_bytes)) {
        fail_on_missing_package("R.utils")
        n_lines <- tryCatch(suppressWarnings(R.utils::countLines(file)), 
            error = function(e) 0)
        n_bytes <- ifelse(file.exists(file), file.info(file)$size, 
            0)
        if (n_lines >= max_lines || n_bytes >= max_bytes) {
            log_trace("lines: %s, max_lines: %s, bytes: %s, max_bytes: %s", 
                n_lines, max_lines, n_bytes, max_bytes, namespace = ".logger")
            log_trace("lines >= max_lines || bytes >= max_bytes: %s", 
                n_lines >= max_lines || n_bytes >= max_bytes, 
                namespace = ".logger")
            for (i in max_files:1) {
                if (i == 1) {
                  log_trace("killing the main file: %s", file, 
                    namespace = ".logger")
                  unlink(file)
                }
                else {
                  new <- paste(file, i - 1, sep = ".")
                  if (i == 2) {
                    old <- file
                  }
                  else {
                    old <- paste(file, i - 2, sep = ".")
                  }
                  if (file.exists(old)) {
                    log_trace("renaming %s to %s", old, new, 
                      namespace = ".logger")
                    file.rename(old, new)
                  }
                  if (i > max_files) {
                    log_trace("killing the file with too many rotations: %s", 
                      new, namespace = ".logger")
                    unlink(new)
                  }
                }
            }
        }
    }
    log_trace("logging %s to %s", shQuote(lines), file, namespace = ".logger")
    cat(lines, sep = "\n", file = file, append = append)
}
<bytecode: 0x5f8022b5b078>
<environment: 0x5f8022b602a8>

But I'm open to solving this more elegantly.

@hadley
Copy link
Collaborator Author

hadley commented Aug 5, 2024

Oh I missed where it was used. If you want to keep this behaviour, I think it would be a little easier to capture it when you use log_appender() etc. Alternatively it might be easier to keep the current behaviour but just set the generator attribute in a way that doesn't interfere with code search. I'll think about this a bit more.

@daroczig
Copy link
Owner

daroczig commented Aug 5, 2024

Usually, the generator is defined via deparse(match.call()), but you are right that it might be possible to capture how the fn was actually called one level up to log_config_setter (as that is being called from e.g. log_appender). I'll try to play with this tomorrow 👍

@hadley
Copy link
Collaborator Author

hadley commented Aug 5, 2024

A simpler approach might just be to adopt a syntax like this:

generator <- function(x) attr(x, "generator")
`generator<-` <- function(x, value) {
  attr(x, "generator") <- value
  x
}

appender_void <- function(lines) {}
generator(appender_void) <- quote(appender_void())

@daroczig
Copy link
Owner

daroczig commented Aug 6, 2024

I've spent some time trying to capture the generator in log_config_setter (basically via match.call(definition = sys.function(-1), call = sys.call(-1))[[2]]), but failed miserably with edge cases e.g. when calling log_appender in do.call, so I think we need to keep tracking the generator close to the function.

The above generator setter/getter approach works for me, although I am not sure yet how to use that elegantly with the factory functions, such as appender_file that returns a function now -- should it need to store the fn in a variable, call the generator setter, and then return on a 3rd line?

@hadley
Copy link
Collaborator Author

hadley commented Aug 7, 2024

I'll take a pass through with the generator approach so you can see what you think. It might possibly also need a set_generator() function so you can also use inline.

@hadley hadley closed this Aug 7, 2024
@hadley hadley deleted the drop-generator branch August 7, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants