-
Notifications
You must be signed in to change notification settings - Fork 28
feat(core,logging): attach location/dependency where wasm_pkg_core::wit::resolve_dependencies failed
#179
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
feat(core,logging): attach location/dependency where wasm_pkg_core::wit::resolve_dependencies failed
#179
Conversation
clippy cleand up logging typos ./crates --write-changes
a8e8464 to
f5126b1
Compare
lann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. Some style and error message suggestions.
crates/wasm-pkg-common/src/config.rs
Outdated
| return Some(reg); | ||
| } | ||
|
|
||
| self.fallback_namespace_registries.get(package.namespace()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge problem but the previous if/else if/else structure seems like more typical rust code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert the style, I had a hard time understanding the else if let Some(_) chains because they all did things slightly differently and it made it tricky to reason about without a consistent corollary.
Only after I broke it down did I realize it was really the same lookup for two different fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For code golf's sake the function could be rewritten as this:
pub fn resolve_registry(&self, package: &PackageRef) -> Option<&Registry> {
let namespace = package.namespace();
// look in `self.package_registry_overrides `
// then in `self.namespace_registries`
self.package_registry_overrides
.get(package)
.or_else(|| self.namespace_registries.get(namespace))
.map(|pkg| pkg.registry())
.or(self.default_registry.as_ref())
.or_else(|| self.fallback_namespace_registries.get(namespace))
}EDIT: I went with a simpler (hopefully more rusty) approach.
Co-authored-by: Lann <[email protected]>
Co-authored-by: Lann <[email protected]>
Co-authored-by: Lann <[email protected]>
Co-authored-by: Lann <[email protected]>
Co-authored-by: Lann <[email protected]>
lann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates
Summary
wkg wit buildcurrently returns an unhelpful report when path resolution fails duringresolve_dependencies:In the case above,
pkg3was the cause of the failure with logging failing to show it.Changes
Displayimpl forDependencyresolve_dependenciesnow attaches paths/dependencies to the relevantanyhow::Error:cargo clippy --fixtypos ./crates --write-changes