Emit more specific popup for metadata fail with cairo version mismatch - #1331
Emit more specific popup for metadata fail with cairo version mismatch#1331maciektr wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| pub fn notify_metadata_failed(&self) { | ||
| pub fn notify_metadata_failed(&self, message: &str) { |
There was a problem hiding this comment.
the function no longer signals only failed metadata
There was a problem hiding this comment.
I don't think I understand what you are trying to say 🤔 Invalid cairo is still a metadata fail.
There was a problem hiding this comment.
I mean read what the code does right now - it doesn't send metadata failure message. It sends any message you pass it
| pub const SCARB_TOML: &str = "Scarb.toml"; | ||
| pub const SCARB_METADATA_FAILED_MESSAGE: &str = | ||
| "`scarb metadata` failed. Check if your project builds correctly via `scarb build`."; | ||
| pub const SCARB_METADATA_CAIRO_VERSION_MISMATCH_MESSAGE: &str = "`scarb metadata` failed due to a Cairo version requirement mismatch. The Cairo version \ |
There was a problem hiding this comment.
it's not used in this file, it shouldn't be here probably
| } | ||
|
|
||
| fn scarb_metadata_message_is_cairo_version_mismatch(message: &ScarbMetadataMessage) -> bool { | ||
| match message { |
There was a problem hiding this comment.
[nit]: i'd rather have if let chain here
| /// | ||
| /// When the failure is caused by a Cairo version requirement mismatch, returns a dedicated | ||
| /// message pointing the user at the root cause instead of the generic one. | ||
| pub fn scarb_metadata_failed_message(messages: &[ScarbMetadataMessage]) -> &'static str { |
There was a problem hiding this comment.
ScarbMetadataMessage prescence doesn't mean that a metadata failed - it can contain diagnostics only which are non fatal. this function can easily lead to misunderstanding of the metadata internals
| // always inform the user with a popup - the message is tailored to the | ||
| // cause (e.g. a Cairo version mismatch) when we can recognize it. | ||
| // Located manifest diagnostics are additionally surfaced inline. | ||
| self.scarb_toolchain.notify_metadata_failed(scarb_metadata_failed_message( |
There was a problem hiding this comment.
This can probably be done easily inside metadata, exposed in invocation result somehow and not leak metadata handling logic outside to project layer.
| if messages.iter().any(scarb_metadata_message_is_cairo_version_mismatch) { | ||
| SCARB_METADATA_CAIRO_VERSION_MISMATCH_MESSAGE | ||
| } else { | ||
| SCARB_METADATA_FAILED_MESSAGE |
There was a problem hiding this comment.
Correct me if im wrong, but not all metadata messages are strictly "fails".
| pub const SCARB_METADATA_FAILED_MESSAGE: &str = | ||
| "`scarb metadata` failed. Check if your project builds correctly via `scarb build`."; | ||
| pub const SCARB_METADATA_CAIRO_VERSION_MISMATCH_MESSAGE: &str = "`scarb metadata` failed due to a Cairo version requirement mismatch. The Cairo version \ |
There was a problem hiding this comment.
nit: i'd move those 2 to scarb_manifest_diagnostics
3 participants
The cairo-version field in manifest may require different scarb/cairo toolchain version than currently available. In this case, emit a more specific popup instead of generic "metadata failed" error