-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Logging and error handling for location utils #12258
Changes from 9 commits
398bb85
b59a667
6885724
79986c8
7fa9319
4250e1f
f584923
ef25255
3462fbe
d5cffea
c436978
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,8 +82,8 @@ export function getProjectAndFeedIdFromInputParam(inputParam: string): any { | |
} | ||
|
||
export function getProjectAndFeedIdFromInput(feedProject: string): any { | ||
var projectId = null; | ||
var feedId = feedProject; | ||
let projectId = null; | ||
let feedId = feedProject; | ||
if(feedProject && feedProject.includes("/")) { | ||
const feedProjectParts = feedProject.split("/"); | ||
projectId = feedProjectParts[0] || null; | ||
|
@@ -95,3 +95,31 @@ export function getProjectAndFeedIdFromInput(feedProject: string): any { | |
projectId: projectId | ||
} | ||
} | ||
|
||
export enum LogType { | ||
debug, | ||
warning, | ||
error | ||
} | ||
|
||
function log(message: string, logType: LogType) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason no to default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No particular reason. This will only be called by the exported logError function below so I'll leave it as is |
||
if (logType === LogType.warning) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For branching behavior for enums I typically use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
tl.warning(message); | ||
} else if (logType === LogType.error) { | ||
tl.error(message); | ||
} else { | ||
tl.debug(message); | ||
} | ||
} | ||
|
||
/** | ||
* Logs the error instead of throwing. | ||
*/ | ||
export function logError(error: any, logType: LogType = LogType.debug) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider talking to the pipelines team to see if we can add behavior to internal.ts |
||
if (error instanceof Error) { | ||
if (error.message) { log(error.message, logType); } | ||
if (error.stack) { log(error.stack, LogType.debug); } // Log stack always as debug | ||
} else { | ||
log(`Error: ${error}`, logType); | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Why can't use the logError function here?
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.
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.