improve error message when using invalid path for local binary artifacts #4129
Conversation
tomerd
force-pushed
the
radar/84365161
branch
from
5eb9760 to
fb4b061
Compare
motivation: clearer, more actionable error message changes: * catch invalid relative path for binary artifacts and emit a clear diagnostics * overall improve validation of binary targets path and urls * add tests rdar://84365161
tomerd
force-pushed
the
radar/84365161
branch
from
fb4b061 to
df41ada
Compare
|
@swift-ci please smoke test |
tomerd
added
the
ready
| } | ||
|
|
||
| guard let url = url.spm_chuzzle(), !url.isEmpty else { | ||
| observabilityScope.emit(.invalidBinaryURL(url: url, targetName: target.name)) |
There was a problem hiding this comment.
I think each error case should have some distinction, e.g. this should say that the URL is empty. Just makes it easier when someone unexpectedly hits one of these, we won't have to guess which case they are encountering. Similar for invalidLocalBinaryPath.
There was a problem hiding this comment.
thanks @neonichu we could add a special case for empty vs. invalid, tho we do print the bad URL/Path to make it obvious. wdyt?
There was a problem hiding this comment.
Ah sorry, I missed that we are printing the URL now, that should be sufficient here.
| continue | ||
| } | ||
|
|
||
| guard let relativePath = try? RelativePath(validating: path) else { |
There was a problem hiding this comment.
Should we make this a do-catch and include the underlying error?
There was a problem hiding this comment.
this is intentional - the underlying error in this case would be a generic "the path is an invalid relative path" which we fine-tune to invalidLocalBinaryPath which explains that this must be relative to the project root
There was a problem hiding this comment.
Right, the question is what happens if the underlying error ever changes and includes useful info. I guess that's not super likely in this particular case.
There was a problem hiding this comment.
right, that was my thinking here as it basically validates that the value is valid relative path
motivation: clearer, more actionable error message
changes:
rdar://84365161