-
Notifications
You must be signed in to change notification settings - Fork 9
Description
jsonc-parser/src/parse_to_value.rs
Line 18 in cf46d10
pub fn parse_to_value<'a>(text: &'a str, options: &ParseOptions) -> Result<Option<JsonValue<'a>>, ParseError> { |
When originally implemented in May 2020 (as-is still currently the case), the AST value is acquired as an Option which then transforms the inner value via a .map()
call + separate match method:
jsonc-parser/src/parse_to_value.rs
Line 28 in cf46d10
Ok(value.map(handle_value)) |
Is there a valid use-case scenario where one would expect this method to return None
? I can see that it is coming from under the hood from parsing the AST with related tests for this expectation:
jsonc-parser/src/parse_to_ast.rs
Lines 523 to 542 in cf46d10
#[test] | |
fn it_should_not_include_tokens_by_default() { | |
let result = parse_to_ast("{}", &Default::default(), &Default::default()).unwrap(); | |
assert!(result.tokens.is_none()); | |
} | |
#[test] | |
fn it_should_include_tokens_when_specified() { | |
let result = parse_to_ast( | |
"{}", | |
&CollectOptions { | |
tokens: true, | |
..Default::default() | |
}, | |
&Default::default(), | |
) | |
.unwrap(); | |
let tokens = result.tokens.unwrap(); | |
assert_eq!(tokens.len(), 2); | |
} |
However for these other parsing methods like parse_to_value()
there is no CollectOptions
config which probably doesn't make much sense to have as a value of JsonValue
is expected, otherwise an error?
So when would None
be relevant in this context to warrant the result wrapping JsonValue
in an option? Or was this an accident in hindsight? (similar to the related parse_to_serde_value()
that was implemented in April 2021)
I only ask as most other format parsing crates I've come across so far (at least those returning a serde value) return a result with the value only. I understand it'd be a breaking change to resolve for jsonc-parser
, just curious if this decision is intentional and serves a purpose.
jsonc-parser/src/parse_to_value.rs
Lines 141 to 145 in cf46d10
#[test] | |
fn it_should_parse_empty() { | |
let value = parse_to_value("", &Default::default()).unwrap(); | |
assert!(value.is_none()); | |
} |
For reference, the equivalent test for serde-json
(tested methods uses from_str()
and from_value()
) parses .""
into an empty String
(and also it's associated Value
variant)
EDIT: The serde-json
test referenced is not the equivalent input(""
vs "\"\""
), my mistake. Parsing an empty string ""
with serde-json
instead emits an error similar to this failure test.