You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Aug 31, 2023. It is now read-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The first attempt of the configuration of the rules tried to be too smart (using IndexMap) but it wasn't well designed and it couldn't be extended easily and as a consequence we have rules.js.rules, which was very redundant.
Fortunately flatten directive form serde seems to fit the bill here, and it works as expected.
Future optimization
At the moment the enabled/disabled rules are always computed every time we get a diagnostic from a rule. Same for when we have to create an AnalysisFilter. This is not efficient. We should look into ways to cache this information, although this would require some work around lifetimes, because RuleFilter only works with references and caching them might only be possible if we use the lifetime coming App.
Test Plan
Updated the tests, and now it all passes! We will need to change the configuration once this lands.
The reason will be displayed to describe this comment to others. Learn more.
Originally the point of using IndexMap was to preserve the ordering of the rules in the original file if the configuration gets re-serialized, this is no longer the case now and the rules will always get printed in the order defined in rules.rs. Would it have been possible to use #[serde(flatten)] instead to inline the rule map into its parent object ?
Originally the point of using IndexMap was to preserve the ordering of the rules in the original file if the configuration gets re-serialized, this is no longer the case now and the rules will always get printed in the order defined in rules.rs. Would it have been possible to use #[serde(flatten)] instead to inline the rule map into its parent object ?
True, but unfortunately I bumped into some hiccups.
I tried different approaches:
using an enum for rules, something like rules: RuleEnume::Js(IndexMap) with a combination of #[serde(untagged)] but unfortunately it didn't workout as I wanted;
I evaluted #[serde(flatten)], but unfortunately doesn't work with deny_unknown_fields, so I am not sure if will workout. I could give it a shot. From the documentation:
Note: flatten is not supported in combination with structs that use deny_unknown_fields. Neither the outer nor inner flattened struct should use that attribute.
I evaluted #[serde(flatten)], but unfortunately doesn't work with deny_unknown_fields, so I am not sure if will workout. I could give it a shot. From the documentation:
Note: flatten is not supported in combination with structs that use deny_unknown_fields. Neither the outer nor inner flattened struct should use that attribute.
I though the lack of support for deny_unknown_fields wouldn't be a problem since the generated deserialize_*_rules functions are checking for unknown fields anyway
I evaluted #[serde(flatten)], but unfortunately doesn't work with deny_unknown_fields, so I am not sure if will workout. I could give it a shot. From the documentation:
Note: flatten is not supported in combination with structs that use deny_unknown_fields. Neither the outer nor inner flattened struct should use that attribute.
I though the lack of support for deny_unknown_fields wouldn't be a problem since the generated deserialize_*_rules functions are checking for unknown fields anyway
PR updated. It now uses flatten and it seems to work as we wanted.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3015
The first attempt of the configuration of the rules tried to be too smart (using
IndexMap) but it wasn't well designed and it couldn't be extended easily and as a consequence we haverules.js.rules, which was very redundant.Fortunately
flattendirective formserdeseems to fit the bill here, and it works as expected.Future optimization
At the moment the enabled/disabled rules are always computed every time we get a diagnostic from a rule. Same for when we have to create an
AnalysisFilter. This is not efficient. We should look into ways to cache this information, although this would require some work around lifetimes, becauseRuleFilteronly works with references and caching them might only be possible if we use the lifetime comingApp.Test Plan
Updated the tests, and now it all passes! We will need to change the configuration once this lands.