Conversation
This reverts commit 6080fd1.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
==========================================
- Coverage 95.13% 95.12% -0.01%
==========================================
Files 36 36
Lines 4930 5002 +72
Branches 4930 5002 +72
==========================================
+ Hits 4690 4758 +68
- Misses 120 123 +3
- Partials 120 121 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
Good work! This will definitely make life easier in the long run.
I've requested changes because I think you should qualify types fully inside the macro definition (std::fmt::Display etc.) so you don't need to import them everywhere you're using that. Besides that, I've just added some other small comments.
I'm in two minds about whether or not it's a good idea to have ID types implement Deserialize, because I can imagine down the line someone wanting to add a new CSV file for something agent-related (or whatever) and sticking an AgentID field in a struct directly rather than getting it from the set of agent IDs, which would kind of circumvent the validation. We'll be converting from *Raw structs anyway so we can construct ID types manually at that point if needed. I suppose by a similar logic, maybe we don't want to implement From<&str> etc. as that's another potential footgun (we could enable it for testing only). I'm probably overthinking it now... Anyway these are always things we can change later if we feel like it.
| fn get_id(&self, id: &str) -> Result<ID> { | ||
| let found = self | ||
| .get(id) | ||
| .with_context(|| format!("Unknown ID {id} found"))?; | ||
| Ok(Rc::clone(id)) | ||
| Ok(found.clone()) | ||
| } | ||
|
|
||
| fn check_id(&self, id: &ID) -> Result<ID> { |
There was a problem hiding this comment.
I'm wondering about the naming, because check_id also returns an ID. How about renaming check_id to get_id and get_id to get_id_by_str or something?
On a separate note, I'm in two minds about whether or not it was a good idea to have get_id() clone the IDs when returning them. Callers can always do that separately if they want. But I guess both ways are basically fine and changing it will mean more churn (though it would make it consistent with your new implementation for HasID).
There was a problem hiding this comment.
I've renamed the functions as suggested. As for cloning the IDs, you're probably right but I can't face changing this right now 😂
There was a problem hiding this comment.
Fair enough! Maybe open an issue for it and we can do it some way down the road if we can be bothered
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
Looks good! Let's merge. Onwards to MUSE's glorious typesafe future 🚀
| impl<T> IDLike for T where | ||
| T: Eq + std::hash::Hash + std::borrow::Borrow<str> + Clone + std::fmt::Display | ||
| { | ||
| } |
There was a problem hiding this comment.
I'm not a fan of what the formatter's done here. Yuck
2 participants
Description
Have fun
Fixes #144
Type of change
Key checklist
$ cargo test$ cargo docFurther checks