Skip to content

update types for heapdump#23891

Closed
mishelen wants to merge 1 commit intoDefinitelyTyped:masterfrom
mishelen:patch-1
Closed

update types for heapdump#23891
mishelen wants to merge 1 commit intoDefinitelyTyped:masterfrom
mishelen:patch-1

Conversation

@mishelen
Copy link
Copy Markdown

  • all arguments are optional
  • callback also return name of file

see package usage

- all arguments are optional
- callback also return name of file
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped

export function writeSnapshot(dumpFileName: string, callback: (err?: Error) => void): void;
export function writeSnapshot(dumpFileName?: string, callback?: (err?: Error, filename?: string) => void): void;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the callback always returns the name of the file, this should be (err: Error | undefined, filename: string) => void.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good rectification

@ghost
Copy link
Copy Markdown

ghost commented Feb 26, 2018

@mishelen Please address comments from the code reviewers.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Feb 26, 2018

@mishelen Thank you for submitting this PR!

🔔 @weekens - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Feb 26, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

@mishelen One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

Copy link
Copy Markdown
Author

@mishelen mishelen left a comment

Choose a reason for hiding this comment

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

Ok

Copy link
Copy Markdown
Author

@mishelen mishelen left a comment

Choose a reason for hiding this comment

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

Ok

@atd-schubert
Copy link
Copy Markdown
Contributor

I also wanted to make a PR on the typings for this library, but take a look at open PRs first.

According to the source-code of heapdump the error is of type Error | null.

In addition, according to the usage section of the README.md there is also the possibility to call the function without a filename. @Andy-MS should I create another PR for that? Do you @mishelen put into your PR?

The source-code should be:

export function writeSnapshot(dumpFileName?: string, callback?: (err: Error | null, filename: string) => void): void;
export function writeSnapshot(callback: (err: Error | null, filename: string) => void): void;

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Apr 14, 2018

Thanks for your contribution. This PR has unaddressed comments and can not be merged in at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the master branch.

@mhegazy mhegazy closed this Apr 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Revision needed This PR needs code changes before it can be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants