Add support overriding block id/tag/hash with a named fork binding
#2475
Conversation
kkawula
requested review from
ksew1,
integraledelebesgue,
cptartur and
ddoktorski
as code owners
kkawula
changed the title
block id/tag/hash with a named fork binding
| } else { | ||
| Err(Diagnostic { | ||
| message: $crate::utils::format_error_message(&messages), | ||
| severity: messages.into_iter().fold(Severity::Warning, |acc, x| $crate::utils::higher_severity(acc, x.severity)) |
There was a problem hiding this comment.
Nit but I think this could just be reduce since this branch will only execute if there is at least one message in messages.
There was a problem hiding this comment.
I suppose that fold will be more idiomatic in this case, we do not have to think about that there always will be at least one member in the list, we can avoid doing uwrap etc.
There was a problem hiding this comment.
Docs are blocked by #1533, page about forks is reconstructed there. I'll wait till they're done
There was a problem hiding this comment.
Nit: Please rename this x to more descriptive name
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! branch { |
There was a problem hiding this comment.
I don't know why this is a macro. It would be more readable as a function.
pub fn branch(results: Vec<Result<String, Diagnostic>>) -> Result<String, Diagnostic> {
let mut messages: Vec<Diagnostic> = Vec::new();
for result in results {
match result {
Ok(val) => return Ok(val),
Err(err) => messages.push(err),
}
}
Err(Diagnostic {
message: format_error_message(&messages),
severity: messages.iter().fold(Severity::Warning, |acc, diagnostic| {
higher_severity(acc, diagnostic.severity)
}),
})
}There was a problem hiding this comment.
I see it now. At lease give a comment that this is way as it is not obvious to caller
There was a problem hiding this comment.
Still can be done with function though
pub fn branch(results: Vec<Box<dyn Fn() -> Result<String, Diagnostic>>>) -> Result<String, Diagnostic> {
let mut messages: Vec<Diagnostic> = Vec::new();
for result in results {
match result() {
Ok(val) => return Ok(val),
Err(err) => messages.push(err),
}
}
Err(Diagnostic {
message: format_error_message(&messages),
severity: messages.iter().fold(Severity::Warning, |acc, diagnostic| {
higher_severity(acc, diagnostic.severity)
}),
})
}There was a problem hiding this comment.
I'll stay with the macro because it has better readability
There was a problem hiding this comment.
No problem 😄. But I think you are mistaking verbosity for readability.
| "tag".to_string(), | ||
| "latest".to_string(), |
There was a problem hiding this comment.
I'd change it to something stable. Let's use specific block number, just different that it is used to override in test.
| @@ -1,30 +1,55 @@ | |||
| use cairo_lang_macro::{Diagnostic, Severity}; | |||
| use indoc::formatdoc; | |||
|
|
|||
| fn higher_severity(a: Severity, b: Severity) -> Severity { | |||
| pub fn higher_severity(a: Severity, b: Severity) -> Severity { | |||
There was a problem hiding this comment.
This has to be changed because the macro uses this function
Closes #2450
Introduced changes
Checklist
CHANGELOG.md