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

refactor(deserialize): generic config deserialization #683

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Nov 6, 2023

Summary

This PR is an application of #620.
In contrast to #620, this proposal is closer to the implementation of serde.
In particular, the visitors are dedicated instances that can hold a state. This allows to support more complex use cases such as deserializing the configuration based on a rule name (The rule name is held by the visitor).

The system relies on 3 main traits:

  • Deserializable that allows to deserialize a type (this is analog to the Deserialize trait of serde).
  • DeserializableValue that allows deserializing a value. This is the trait implemented to support the deserialization of a new data format
  • DeserializationVisitor which allows visiting a value (array, map, boolean ,string, ...)

One advantage of separating Deserializable and DeserializationVisitor is the possibility of reusing a visitor for several types, or just implementing Deserializable by reusing other derializable types.

Tasks:

  • Documentation
  • Macros for auto-implement Deserializable (future)

Test Plan

CI should pass.

@Conaclos Conaclos temporarily deployed to Website deployment November 6, 2023 15:37 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Nov 6, 2023
@Conaclos Conaclos changed the title reafctor(deserialize): geenric config deserialization reafctor(deserialize): generic config deserialization Nov 6, 2023
@Conaclos Conaclos changed the title reafctor(deserialize): generic config deserialization refactor(deserialize): generic config deserialization Nov 6, 2023
@Conaclos Conaclos temporarily deployed to Website deployment November 7, 2023 13:21 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 7, 2023 13:32 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Tooling Area: internal tools label Nov 7, 2023
Copy link
Contributor

github-actions bot commented Nov 7, 2023

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 592 592 0
Failed 70 70 0
Panics 0 0 0
Coverage 89.43% 89.43% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13454 13454 0
Failed 4190 4190 0
Panics 2 2 0
Coverage 76.24% 76.24% 0.00%

@Conaclos Conaclos temporarily deployed to Website deployment November 7, 2023 16:33 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Formatter Area: formatter label Nov 7, 2023
@Conaclos Conaclos temporarily deployed to Website deployment November 7, 2023 17:08 — with GitHub Actions Inactive
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I like this refactor. I am looking forward to getting this shipped. I left some suggestions and questions, feel free to address them or not.

Here are some things that we should do (now or after we merge the PR):

  • Documentation: things now require a different implementation, especially for custom types where we require the usage of some external visitors (Unsize... types for example). Let's craft good documentation with some examples, especially how to deserialize enums, numbers and custom types, e.g. StringSet
  • Explain when to use Deserializable and when to use Visitor. Even though we took a lot of inspiration from serde, we can't assume that people know how to use it, especially new contributors that want to create rules with options.

crates/biome_deserialize/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/biome_deserialize/src/json.rs Outdated Show resolved Hide resolved
crates/biome_deserialize/src/json.rs Outdated Show resolved Hide resolved
crates/biome_deserialize/src/json.rs Outdated Show resolved Hide resolved
crates/biome_deserialize/src/json.rs Outdated Show resolved Hide resolved
crates/biome_formatter/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_formatter/src/lib.rs Outdated Show resolved Hide resolved
@Conaclos Conaclos temporarily deployed to Website deployment November 10, 2023 15:04 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 10, 2023 15:48 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 10, 2023 16:33 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 10, 2023 16:42 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/deser branch 2 times, most recently from dfe94a3 to 99d2aac Compare November 12, 2023 15:42
@Conaclos Conaclos temporarily deployed to Website deployment November 12, 2023 15:42 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 12, 2023 23:11 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 12, 2023 23:35 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 12, 2023 23:46 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment November 13, 2023 00:04 — with GitHub Actions Inactive
@Conaclos
Copy link
Member Author

@ematipico I think I addressed all your concerns., but one:

I am still unsure what to do when a member of an array or a map has a parsing error or a deserialization error.

I see several possibilities:

  1. ignore members that cannot be deserialized
  2. ignore members that cannot be deserialized for unordered data structures (HashMap and HasSet) and make the deserialization failed for ordered data structures (Vec, IndexSet, IndexMap) if at least one member cannot be deserialized.
  3. make deserilization failed if at least one member cannot be deserialized.
  4. Make the behavior configurable by passing an option to every deseriliazer. This will affect deserialization in the subtree.

I think (1) is the best, or maybe (2) as a tradeoff.
I am not fond of (4) because it adds a fourth parameter to Deserialize::deserialize.
Otheriwe, we could add Deserialize::deserialize_with_options.

@ematipico
Copy link
Member

I also think the first option is the best!

@Conaclos Conaclos merged commit e99387e into main Nov 13, 2023
18 checks passed
@Conaclos Conaclos deleted the conaclos/deser branch November 13, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Formatter Area: formatter A-Linter Area: linter A-LSP Area: language server protocol A-Project Area: project A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants