-
Notifications
You must be signed in to change notification settings - Fork 30
Display tree of srcref/srcfile objects #85
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
base: main
Are you sure you want to change the base?
Conversation
DavisVaughan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't read the code too closely but I did read the docs closely and I really like having this all in one place!
be316eb to
d3e07a8
Compare
d3e07a8 to
3e324c0
Compare
hadley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great and you should feel free to merge whenever. But I'm also happy to discuss more 😄
| #' @param file Optional file path (default: creates temp file) | ||
| #' @return The result of sourcing the code with keep.source = TRUE | ||
| #' @noRd | ||
| with_srcref <- function(code, env = parent.frame(), file = NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to save to a file? Doesn't (eval(parse_with_srcref(code))) give you an object with srcref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just copied that util from rlang as is. But there are differences between the two approaches, parse doesn't produce the same kind of srcref as source. I get expectation failures when I change with_srcref to use parse.
|
|
||
| test_that("src() shows quoted function with nested body", { | ||
| expect_snapshot({ | ||
| with_srcref("x <- quote(function() {})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the assignment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this gets assigned in the current environment and is inspected just below.
tests/testthat/test-src.R
Outdated
| test_that("src() shows closure with srcref and wholeSrcref", { | ||
| expect_snapshot({ | ||
| f <- simple_function_with_srcref() | ||
| scrub_src(src(f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this works interactively:
f <- function() {
x + 1 # comment
{}
}
scrub_src(src(f))But not
expect_snapshot({
f <- function() {
x + 1 # comment
{}
}
scrub_src(src(f))
})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm both work for me
|
TODO:
|
gaborcsardi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! I didn't look at the code much, only the docs and ran some examples, but the docs are just as valuable as the code, anyway. :)
Left some minor comments about the docs.
| #' optionally, the parsed-line numbers if `#line` directives were used. | ||
| #' | ||
| #' Lengths of 4, 6, or 8 are allowed: | ||
| #' - 4: basic (first_line, first_byte, last_line, last_byte) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not too hard to deduce, but maybe worth noting that first_byte and last_byte are within the line. (Right?)
| #' there is no support for encodings other than UTF-8. | ||
| #' | ||
| #' The srcref columns are right-boundary positions, meaning that for an | ||
| #' expression starting at the start of the file, the column will be 1. Note that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean at the start of a line? I.e. bytes and columns are counted from the beginning of the line, right?
| #' location, for example from a temporary file or generated file to the original | ||
| #' location on disk. | ||
| #' | ||
| #' Called by `install.packages()` when installing a _source_ package with `keep.source.pkgs` set to `TRUE` (see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Created" by install.packages()... ?
| #' - `Enc`: The encoding of output lines. Used by `getSrcLines()`, which | ||
| #' calls `iconv()` when `Enc` does not match `encoding`. | ||
| #' | ||
| #' - `parseData` (optional): Parser information saved when `keep.source.data` is #' set to `TRUE`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline needed before the #'
|
Btw. do you know when the source refs on |
This PR:
The tree viewer is helpful to get a grasp of the complex web of objects created by the parser:
Principles of the tree view:
body(),attr(), or[[).srcreforwholeSrcrefattributes on lists).parsedis only shown if different fromlocation(due to a#linedirective)Here is an example of
src()showing its own srcrefs tree: