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

Add feature inspector to FML #5827

Merged
merged 3 commits into from Sep 20, 2023
Merged

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Sep 18, 2023

Fixes EXP-3793.

This PR turned out larger than I was expecting:

  • changed the IR of FeatureManifest from Vec<_> to BTreeMap<String, _>. This is long overdue, but I've been resisting doing it until now.
  • added ref support to the FMLClient.
  • refactor client.rs into client/mod.rs and associated files.
  • serde_json and serde_yaml don't support line/column numbers. Worse, while serde_json can be configured to preserve insertion order, it's not clear we're not clobbering the order some place in the code. So we need to provide hints on where the error messages are based upon the paths through the JSON.
  • added a find_index method to get the (line, col) number for where the errors occur.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@jhugman jhugman force-pushed the jhugman/exp-3793-add-feature-inspector branch from 5ef635f to 5b7a653 Compare September 18, 2023 18:17
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023

Codecov Report

Patch coverage: 36.03% and project coverage change: -0.04% ⚠️

Comparison is base (9d7c703) 36.99% compared to head (c80aa06) 36.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5827      +/-   ##
==========================================
- Coverage   36.99%   36.96%   -0.04%     
==========================================
  Files         343      346       +3     
  Lines       33114    33289     +175     
==========================================
+ Hits        12251    12304      +53     
- Misses      20863    20985     +122     
Files Changed Coverage Δ
components/support/nimbus-fml/src/client/config.rs 0.00% <0.00%> (ø)
...onents/support/nimbus-fml/src/client/descriptor.rs 0.00% <0.00%> (ø)
...ponents/support/nimbus-fml/src/client/inspector.rs 0.00% <0.00%> (ø)
components/support/nimbus-fml/src/client/mod.rs 0.00% <0.00%> (ø)
components/support/nimbus-fml/src/error.rs 29.82% <93.33%> (+18.46%) ⬆️
...port/nimbus-fml/src/intermediate_representation.rs 95.90% <98.59%> (+0.11%) ⬆️
...mponents/support/nimbus-fml/src/defaults_merger.rs 95.37% <100.00%> (+0.03%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jhugman jhugman changed the title Jhugman/exp 3793 add feature inspector Add feature inspector to FML Sep 19, 2023
Copy link
Contributor Author

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

Self review/guide for reviewers.

@@ -0,0 +1,91 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code has been reviewed before; this moves it all into a file called client/descriptor.rs.

}

pub fn validate_default_by_typ(
&self,
path: &str,
type_ref: &TypeRef,
default: &Value,
literals: &Vec<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work in this method is to add the literals vec, a list of tokens leading from the root to where an error is detected.

The literals vec is then reported in errors, which are then unpacked in the FmlClient.

Several improvements suggest themselves:

  • remove the FMLError::ValidationError from this method. Those problems are likely detected in another method.
  • use the preserve_order feature of the serde_json, and add more to the literals vec. This will improve the accuracy of the error placement; however this should be done in a different issue— because it may need work in the DefaultsMerger.
  • Process the invalid property detection for features here, perhaps by translating the FeatureDef into ObjectDef (currently this is done in DefaultsMerger).
  • Split this feature validation into a smaller more managable file/set of methods; this would make reviewing/editing the error messages easier, at the very least.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much for this explanation!! 🙏

}

pub fn get_feature(&self, nm: &str) -> Option<&FeatureDef> {
self.iter_feature_defs().find(|f| f.name() == nm)
self.feature_defs.get(nm)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. O(1) instead of O(n).

@jhugman jhugman force-pushed the jhugman/exp-3793-add-feature-inspector branch 2 times, most recently from ef36cd4 to 9b3a9de Compare September 19, 2023 13:25
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

I hate to ask this, but how complicated would it be to separate the change from Vec<_> to BTreeMap<String, _> from the rest of this? Is it better to land it all in one chunk? (I know it can be tricky and things are interconnected, but figured it was worth asking 😅 )

Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

This looks great! I would ideally like to split it up so that the refactor from Vec<_> to BTreeMap<String, _> is landed first, but I'm also happy to just get this going. Up to you. One little nit/question about the tests, but this LGTM 💯

Comment on lines +10 to +26
pub struct FmlLoaderConfig {
pub cache: Option<String>,
pub refs: HashMap<String, String>,
pub ref_files: Vec<String>,
}

impl From<FmlLoaderConfig> for LoaderConfig {
fn from(value: FmlLoaderConfig) -> Self {
let cwd = std::env::current_dir().expect("Current Working Directory is not set");
let cache = value.cache.map(|v| cwd.join(v));
Self {
cwd,
refs: value.refs.into_iter().collect(),
repo_files: value.ref_files,
cache_dir: cache,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

}

impl FmlFeatureInspector {
fn get_syntax_error(&self, string: &str) -> Result<Value, FmlEditorError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! get_syntax_error and get_semantic_error 👏

}

#[test]
fn test_find_err() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little unsure -- is this test covering the case when there's no error to be found? What about if the path is invalid?

@@ -65,3 +70,18 @@ pub enum ClientError {
}

pub type Result<T, E = FMLError> = std::result::Result<T, E>;

pub(crate) fn did_you_mean(words: HashSet<String>) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 👍

Comment on lines 480 to 482
fm.iter_enum_defs()
.map(|e| &e.name)
.collect::<HashSet<&String>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's much prettier 🍰

}

pub fn validate_default_by_typ(
&self,
path: &str,
type_ref: &TypeRef,
default: &Value,
literals: &Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much for this explanation!! 🙏

@jhugman jhugman force-pushed the jhugman/exp-3793-add-feature-inspector branch from c80aa06 to 59fa04e Compare September 20, 2023 17:03
@jhugman jhugman force-pushed the jhugman/exp-3793-add-feature-inspector branch from 59fa04e to 2ac2d80 Compare September 20, 2023 17:07
@jhugman jhugman added this pull request to the merge queue Sep 20, 2023
Merged via the queue into main with commit fa8129e Sep 20, 2023
16 checks passed
@jhugman jhugman deleted the jhugman/exp-3793-add-feature-inspector branch September 20, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants