Logging and error handling for location utils #12258
Conversation
phil-hodgson
requested review from
bishal-pdMSFT,
damccorm,
thesattiraju,
leantk and
nishubansal
as code owners
satbai
force-pushed
the
users/philho/locationUtilities
branch
from
b55e9a8 to
6885724
Compare
satbai
force-pushed
the
users/philho/locationUtilities
branch
from
e6eb532 to
7fa9319
Compare
Tasks/Common/packaging-common/Strings/resources.resjson/en-US/resources.resjson
Outdated
Show resolved
Hide resolved
| @@ -11,7 +11,12 @@ export async function retryOnException<T>(action: () => Promise<T>, maxTries: nu | |||
| throw error; | |||
| } | |||
| tl.debug(`Attempt failed. Number of tries left: ${maxTries}`); | |||
| tl.debug(JSON.stringify(error)); | |||
| if (error instanceof Error) { | |||
There was a problem hiding this comment.
Why can't use the logError function here?
There was a problem hiding this comment.
It's because I put the helper in packaging-common and this is in artifacts-common. I didn't want to create a dependency between the two common folders.
| error | ||
| } | ||
|
|
||
| function log(message: string, logType: LogType) { |
There was a problem hiding this comment.
Any reason no to default logType to debug?
There was a problem hiding this comment.
No particular reason. This will only be called by the exported logError function below so I'll leave it as is
| } | ||
|
|
||
| function log(message: string, logType: LogType) { | ||
| if (logType === LogType.warning) { |
There was a problem hiding this comment.
For branching behavior for enums I typically use a switch (with a "default: throw UnknownEnum"). That fails faster when a new enum value is added and the branching wasn't updated.
There was a problem hiding this comment.
Switch is a good one. If we add more log types we can change it to a switch but since there are only three options I'll leave it as is for now. Thanks!
| /** | ||
| * Logs the error instead of throwing. | ||
| */ | ||
| export function logError(error: any, logType: LogType = LogType.debug) { |
There was a problem hiding this comment.
Consider talking to the pipelines team to see if we can add behavior to internal.ts
No description provided.