Skip to content
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: add type parameter to warn function #1009

Merged
merged 8 commits into from
May 24, 2021
Merged

Conversation

alicejli
Copy link
Contributor

Adding a type parameter to the warn function in preparation for emitting a DeprecationWarning for deprecated methods/services.(googleapis/gapic-generator-typescript#853)

Fixes #1008 🦕

@alicejli alicejli requested a review from a team as a code owner May 19, 2021 21:00
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2021
Copy link
Contributor

@summer-ji-eng summer-ji-eng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For lint issue, you could run npm run fix and npm run lint

@alicejli alicejli changed the title Add type parameter to warn function feat: add type parameter to warn function May 19, 2021
src/warnings.ts Outdated
@@ -18,7 +18,8 @@ import {isBrowser} from './isbrowser';

const emittedWarnings = new Set<string>();

export function warn(code: string, message: string) {
// Type is the type of warning (e.g. 'DeprecationgWarning', 'ExperimentalWarning', etc.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks for adding the comments.

src/warnings.ts Outdated
Comment on lines 32 to 36
if (type !== 'undefined') {
process.emitWarning(message, type);
} else {
process.emitWarning(message);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@summer-ji-eng summer-ji-eng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave one comment, other than that, LGTM
Thanks for your work.

src/warnings.ts Outdated
@@ -28,6 +29,12 @@ export function warn(code: string, message: string) {
if (isBrowser()) {
console.warn(message);
} else {
process.emitWarning(message);
if (typeof warnType !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit(style): please merge this into one if-else if-else chain to reduce nesting :)

@alicejli alicejli merged commit fef2e7c into googleapis:master May 24, 2021
@alicejli alicejli deleted the warn branch May 24, 2021 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'type' parameter to 'warnings.ts'
3 participants