Fix regression in unmatched globs error message #10595
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
|
Could I ask you to provide an example of the error message before and after in the description? |
| let input_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?; | ||
| let input_digest = | ||
| lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?; |
There was a problem hiding this comment.
Note that this change and most others are not necessary. The only thing we need is the fix for download_file_to_digest and path_globs_to_digest.
But, personally, I think I find this refactor easier to follow and that it has less coercion of types, like no let Result<_, String>. Still, let me know if this is actually an improvement or I should revert to the old.
There was a problem hiding this comment.
The one concern I have about this hunk is that it seems to add two throw() conversions where there were none before. Is it possible to keep those as string errors and then re-use the .map_err() at the bottom? Or is the point of this part that you have to add those two throw()s in order to remove the let res statement?
There was a problem hiding this comment.
I think removing the let res is clearer regardless, so I'm fine if we make this tradeoff.
There was a problem hiding this comment.
This is only part of the refactor. The end effect should be the same, so it's only a question of what's clearer here.
I added comments on the two code blocks that are the actual fixes. Everything else can be reverted if preferred.
Pardon that I didn't make that clearer from the start, and thank you for the prompt feedback :)
|
@cosmicexplorer yes, I updated the PR description. |
| let input_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?; | ||
| let input_digest = | ||
| lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?; |
There was a problem hiding this comment.
The one concern I have about this hunk is that it seems to add two throw() conversions where there were none before. Is it possible to keep those as string errors and then re-use the .map_err() at the bottom? Or is the point of this part that you have to add those two throw()s in order to remove the let res statement?
| let input_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?; | ||
| let input_digest = | ||
| lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?; |
There was a problem hiding this comment.
I think removing the let res is clearer regardless, so I'm fine if we make this tradeoff.
| let digest = store.merge(digests).await.map_err(|e| format!("{:?}", e))?; | ||
| let res: Result<_, String> = Ok(Snapshot::store_directory(&context.core, &digest)); | ||
| res | ||
| let digests = future::try_join_all(digests).await.map_err(|e| throw(&e))?; |
There was a problem hiding this comment.
Is this the part that fixes it (because we don't call format!() on the inside and then throw() on the outside)?
There was a problem hiding this comment.
No, only a refactor.
| async move { | ||
| let key = externs::acquire_key_for(args.pop().unwrap()).map_err(|e| format!("{:?}", e))?; | ||
| let snapshot = context | ||
| .get(DownloadedFile(key)) | ||
| .await | ||
| .map_err(|e| format!("{:?}", e))?; | ||
| let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &snapshot.digest)); | ||
| res | ||
| let key = externs::acquire_key_for(args.pop().unwrap())?; | ||
| let snapshot = context.get(DownloadedFile(key)).await?; | ||
| Ok(Snapshot::store_directory(&core, &snapshot.digest)) | ||
| } | ||
| .map_err(|err: String| throw(&err)) | ||
| .boxed() | ||
| } |
There was a problem hiding this comment.
Actual fix for DownloadFile.
| let key = externs::acquire_key_for(args.pop().unwrap()).map_err(|e| format!("{:?}", e))?; | ||
| let digest = context | ||
| .get(Snapshot(key)) | ||
| .await | ||
| .map_err(|e| format!("{:?}", e))?; | ||
| let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &digest)); | ||
| res | ||
| let key = externs::acquire_key_for(args.pop().unwrap())?; | ||
| let digest = context.get(Snapshot(key)).await?; | ||
| Ok(Snapshot::store_directory(&core, &digest)) | ||
| } | ||
| .map_err(|err: String| throw(&err)) |
There was a problem hiding this comment.
Actual fix for PathGlobs.
| let digest = store.merge(digests).await.map_err(|e| format!("{:?}", e))?; | ||
| let res: Result<_, String> = Ok(Snapshot::store_directory(&context.core, &digest)); | ||
| res | ||
| let digests = future::try_join_all(digests).await.map_err(|e| throw(&e))?; |
There was a problem hiding this comment.
No, only a refactor.
| let input_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?; | ||
| let input_digest = | ||
| lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?; |
There was a problem hiding this comment.
This is only part of the refactor. The end effect should be the same, so it's only a question of what's clearer here.
I added comments on the two code blocks that are the actual fixes. Everything else can be reverted if preferred.
Pardon that I didn't make that clearer from the start, and thank you for the prompt feedback :)
Fixes #10574. We were calling
throw(&format("{:?}"))on an error that was already had typeFailure, meaning that we were converting aFailureto aString, then back to aFailure, which resulted in an overly chatty error message.Before:
After:
[ci skip-build-wheels]