Return a result for ResourceManager::get_resource #282
Conversation
|
@gregtatum just creating a draft here to get early feedback and see if I'm going in the right direction ^^ |
There was a problem hiding this comment.
Yeah this seems to be in the right direction. I did a quick read-through with a few comments. It will need tests. I have some I wrote on some local patches working on some code coverage (that's where this issue came from) but feel free to add your own, or wait a day or two until I get mine online to base it off of.
fluent-resmgr/Cargo.toml
Outdated
| @@ -22,6 +22,7 @@ fluent-fallback = { version = "0.6.0", path = "../fluent-fallback" } | |||
| unic-langid = "0.9" | |||
| elsa = "1.5" | |||
| futures = "0.3" | |||
| thiserror = "1.0.37" | |||
There was a problem hiding this comment.
Suggestion: The version number should match the other thiserror version.
There was a problem hiding this comment.
Is this ok if we start using workspace dependencies to simplify the versions across the crates? See this commit a75bf08. It was released in Rust 1.64.0 a few months ago, it's pretty handy.
There was a problem hiding this comment.
This might be better as a separate PR. I don't know if the Firefox vendoring process works with workspace, and we would need to block on verifying that it would work. I wouldn't want to break the Firefox integration.
|
|
||
| let mut errors = vec![]; | ||
| let msg = bundle.get_message("hello-world").expect("Message exists"); | ||
| let pattern = msg.value().expect("Message has a value"); | ||
| let value = bundle.format_pattern(&pattern, None, &mut errors); |
There was a problem hiding this comment.
Question: These & changes seem unrelated drive-by fixes. I'm assuming since the pattern here is already a reference.
There was a problem hiding this comment.
Yeah, I did a few clippy quick fixes, I thought it wouldn't hurt much 😄
|
@gregtatum I just rebased and fixed the tests you wrote to make them work with my changes. Do you see anything else to do on this? |
There was a problem hiding this comment.
Thanks for the updates on the patch! This is real close!
I'm marking request changes for a few suggestions and nitpicks that I had.
I also found one issue, where the get_bundles method had a change in behavior.
Once you are ready for a re-review please click the little "refresh" icon next to my name in the review, as this will re-trigger review for me. I missed the comments that you had left in my notifications deluge. Setting the review status will make sure that it's in my queue to take a look at.
Cargo.toml
Outdated
| @@ -13,3 +13,6 @@ members = [ | |||
| exclude = [ | |||
| "fluent-cli", | |||
| ] | |||
|
|
|||
| [workspace.dependencies] | |||
| thiserror = "1.0" | |||
There was a problem hiding this comment.
Suggestion: Per my comment earlier, I'd prefer to still use the old way until we can:
- Verify that Firefox vendoring still works.
- Apply these changes to the entire repo.
Feel free to file a follow-up PR with this.
There was a problem hiding this comment.
Makes sense, I'll provide another PR for this so that we are not blocked by it!
| let res = self.get_resource(res_id, &locales[0].to_string()); | ||
| bundle.add_resource(res).unwrap(); | ||
|
|
||
| resource_ids.iter().for_each(|res_id| { |
There was a problem hiding this comment.
Nit: I don't think anywhere else has used the for_each. I think it would be simpler and more idiomatic to use a for loop here.
| bundle.add_resource(res).unwrap(); | ||
|
|
||
| resource_ids.iter().for_each(|res_id| { | ||
| let _ = self |
There was a problem hiding this comment.
Nit: let _ isn't needed here.
There was a problem hiding this comment.
Clippy was unhappy about it, but I switched to a match to avoid this ugly let _
| }); | ||
| }); | ||
|
|
||
| if errors.iter().len() > 0 { |
There was a problem hiding this comment.
Nit: This could be simplified to errors.is_empty().
| }) | ||
| ) -> Result<impl Iterator<Item = FluentBundle<&FluentResource>>, Vec<ResourceManagerError>> | ||
| { | ||
| let mut ptr = 0; |
There was a problem hiding this comment.
Nit: idx is clearer choice here compared to ptr. I think this may have been missed in a rebase.
| let mut errors: Vec<ResourceManagerError> = vec![]; | ||
| let mut res = vec![]; | ||
|
|
||
| while let Some(locale) = locales.get(ptr) { |
There was a problem hiding this comment.
Issue: This changes the behavior of this function from being lazy (where the bundles are loaded on each iteration), to fully evaluating the bundles. The intent for this iterator is to only evaluate the bundle on iteration.
Rather than returning Result<Iterator> this should return a:
impl Iterator<Item = Result< ... >>
There was a problem hiding this comment.
Wooops, sorry about that 🙈
fluent-syntax/Cargo.toml
Outdated
| @@ -27,7 +27,7 @@ include = [ | |||
| [dependencies] | |||
| serde = { version = "1.0", features = ["derive"], optional = true } | |||
| serde_json = { version = "1.0", optional = true } | |||
| thiserror = "1.0" | |||
| thiserror.workspace = true | |||
There was a problem hiding this comment.
Suggestion: See above on the workspace feedback.
See #278 for more context