Skip to content

Fail in the face of parse errors#1

Merged
jeroen merged 1 commit intor-lib:mainfrom
DavisVaughan:feature/parse-errors
Aug 6, 2025
Merged

Fail in the face of parse errors#1
jeroen merged 1 commit intor-lib:mainfrom
DavisVaughan:feature/parse-errors

Conversation

@DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Aug 6, 2025

I feel like if we were to use this then we'd fail early if there are parse errors to avoid weirdness

We get a data frame of info back when there are parse errors which we could use to inform the user about where the problem is

> json_parse_errors('{"a": asdf}')
  error offset length
1     1      6      4
2     4     10      1
> # error 1 = invalid symbol
> # error 4 = value expected

For now I'm not using this

https://github.com/microsoft/node-jsonc-parser/blob/fe330190baba3ba630934d27ea2083638feddadc/src/main.ts#L151

Also worth noting that just yesterday they added startLine and startCharacter to the error output struct, so we'd get that too and it might be easier to show the user info using that info
microsoft/node-jsonc-parser#102

Comment on lines 53 to 61
json_format_text <- function(text, spaces = 4) {
stopifnot(is.character(json))
json <- paste(json, collapse = '\n')
stopifnot(is.character(text))
text <- paste(text, collapse = '\n')
opts <- list(insertSpaces = spaces > 0, tabSize = spaces)
jsonc$call('json_format', json, opts)
if (!is.null(json_parse_errors(text))) {
stop("Can't format when there are existing parse errors.")
}
jsonc$call('json_format', text, opts)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json wasn't actually defined here so this previously failed. i think you meant to use text, and ive standardized all functions to just use text now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right I renamed those variables at some point 🤦

@jeroen jeroen merged commit 6ccf659 into r-lib:main Aug 6, 2025
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