refactor(json_deserailize): remove visit_member_name

[Conaclos]

Summary

• rename visit_member_value to visit_value
• remove visit_member_name

The removal of visit_member_name allowed the removal of has_only_known_keys.
This utility function is now replaced by report_unknown_map_key.
It is called in visit_map when an unknown key is detected.

I also replaced has_only_known_variants with report_unknown_variant.
It is called in visit_value when an unknown variant is found.

Test Plan

Ci should pass.

A CLI snapshot was updated because it now correctly report an unknown rule name.

[github-actions]

Parser conformance results on

js/262

Test result	main count	This PR count	Difference
Total	49701	49701	0
Passed	48720	48720	0
Failed	981	981	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%

[ematipico]

Thank you @Conaclos , could you also explain why you did this refactor? It would be great to have some context

[ematipico]

I am going to block it so I get the notification when you update the PR. Could you please share any info on potential optimisations (memory and/or speed)?

[ematipico]

This crate is meant to create "events" on specific types so the consumer can do what they need on those types. By removing these visitors, we removed the ability to visit them. I don't think that's what we meant. It's like a simplified serde.

While member_name and member_value can arguably be better names, plain_value is more cryptic because it doesn't explain what we are visiting. Value of a key? And what about the keys themselves?

[ematipico]

This is another change that I need help understanding. I should probably wait for your explanation.

[Conaclos]

could you also explain why you did this refactor? It would be great to have some context

Mainly to avoid checking twice if a key is valid. Before this PR we first check if the key is allowed in visit_member_name by linearly traversing the array of allowed keys, and then we match on valid keys in visit_map.

While member_name and member_value can arguably be better names, plain_value is more cryptic because it doesn't explain what we are visiting. Value of a key?

In fact, I found viist_member_value confusing. I first thought that the method visited value associated to a key. In fact, it is used to visit "plain" values (strings, integers, Boolean). Do you have a better name? Do you prefer viist_member_value?

And what about the keys themselves?

I don't see the purpose of viist_member_name since we already visit the key (and its value) in visit_map.

I think we could go beyond by removing the ViisitNode<L> abstraction. I don't understand the value of this abstraction. Instead, we could introduce several JSON Visitor traits such as VisitJsonObject (with methods visit_object, visit_object_member), VisitJsonArray (with methods visit_array, visit_array_member). This could avoid default method implementation with unimplemented(). These new visitors could use high-level types (JsonObjectValue, JsonArrayValue). In the same way that the current implementation, we could provide the utility functions map_to_* to its visitors.

[ematipico]

The purpose of biome_deserialize is to provide an abstraction to use our parsers to parse a file and deserialize it into a Rust type. In the future, we could have a biome.toml or a biome.yml, and we could use the same visitor trait system. Or even a different Rus type (a lock file?) and implement the deserialization.

We had this system in the old Rome (typescript), too.

Mainly to avoid checking twice if a key is valid. Before this PR we first check if the key is allowed in visit_member_name by linearly traversing the array of allowed keys, and then we match on valid keys in visit_map

Because that's how the implementor does it, but that doesn't mean that we are meant to delete the original methods. We just need to not call a method if we can, or we don't need to. You can't know if in the future another implementor will need or not to use that visitor.

I don't see the purpose of viist_member_name since we already visit the key (and its value) in visit_map.

If you don't use it, it doesn't mean it's not needed and doesn't have a purpose. You should see this crate as a library for general purposes, not something specifically tailored over JSON.

The idea is to have:

• maps
• lists
• key of map
• value of map

Then, each language names these types differently, but they are semantically the same. I hope this clarifies a bit things, please let me know if you have more questions

[Conaclos]

The purpose of biome_deserialize is to provide an abstraction to use our parsers to parse a file and deserialize it into a Rust type. In the future, we could have a biome.toml or a biome.yml, and we could use the same visitor trait system. Or even a different Rus type (a lock file?) and implement the deserialization.

Even in the case where we introduce a new config language, I doubt that we will be able to share code that uses VisitNode<L> with a parameterized L. All current code use JSON visitors in cascade. L is always set to 'JsonLanguage'. No code is implemented using VisitNode<L> with a generic L.

• value of map

And value of list? And plain values?

Because that's how the implementor does it, but that doesn't mean that we are meant to delete the original methods. We just need to not call a method if we can, or we don't need to. You can't know if in the future another implementor will need or not to use that visitor.

I might disagree with you on this one. I prefer to add a feature when it is actually used or planned to be used.

[ematipico]

The purpose of biome_deserialize is to provide an abstraction to use our parsers to parse a file and deserialize it into a Rust type. In the future, we could have a biome.toml or a biome.yml, and we could use the same visitor trait system. Or even a different Rus type (a lock file?) and implement the deserialization.

Even in the case where we introduce a new config language, I doubt that we will be able to share code that uses VisitNode<L> with a parameterized L. All current code use JSON visitors in cascade. L is always set to 'JsonLanguage'. No code is implemented using VisitNode<L> with a generic L.

And you never will, as long as we have L; that's the limitation of L, and it's not a limitation of just this crate :)

• value of map

And value of list? And plain values?

Any value, depending on the language we're parsing. TOML or YAML supports more values than JSON. But yes, plain values

Because that's how the implementor does it, but that doesn't mean that we are meant to delete the original methods. We just need to not call a method if we can, or we don't need to. You can't know if in the future another implementor will need or not to use that visitor.

I might disagree with you on this one. I prefer to add a feature when it is actually used or planned to be used.

We could remove what we don't use, not a big deal

[ematipico]

I caught some changes that aren't needed and some other changes should be reverted because they are breaking changes.