-
-
Notifications
You must be signed in to change notification settings - Fork 467
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
refactor(biome_deserialize): add deserilization context #4166
base: next
Are you sure you want to change the base?
Conversation
Parser conformance results onjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
dd0d54c
to
016732b
Compare
CodSpeed Performance ReportMerging #4166 will not alter performanceComparing Summary
|
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.
Usually, breaking changes should be presented with a clear migration path for users.
I know that we don't have a changelog for the crate yet, but maybe we should. As for now, I think we should have at least a migration path in the description of the PR.
Can you point this to |
016732b
to
7d12d72
Compare
7d12d72
to
d45b68f
Compare
2da418c
to
0a7d005
Compare
0a7d005
to
f024c39
Compare
Summary
This is a BREAKING CHANGE for library consumers.
This implements the design proposed in this comment.
The context allows reporting diagnostics and to retrieve the identifier of the root object.
The identifier can be anything. It can notably be the path to the deserialized file: what was requested here.
Note that
deserialize_from_json_str
anddeserialize_from_json_ast
last argument was previously passed asname
to the deserializer.It is now used as identifier.
The
name
of the root deserializer is always set to the empty string.Also I removed the
passthrough_name
derive macro attribute. This attribute was introduced to allow to pass a name between deserializers. It is no longer needed since we havectx.id()
.NOTE: we no longer need this refactor. However, this could be good to merge.
We should check with our library consumers if it is something they still require.
Test Plan
CI must pass.