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.

Sign up for GitHub

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

Jump to bottom

feat(cli) add Deno.systemMemoryInfo() #7350

Merged
merged 16 commits into from
Sep 10, 2020
Merged

feat(cli) add Deno.systemMemoryInfo() #7350

merged 16 commits into from
Sep 10, 2020

Conversation

humancalico
Copy link
Contributor

@humancalico humancalico commented Sep 4, 2020 •
edited
Loading

Based on #6417

@CLAassistant
Copy link

CLAassistant commented Sep 4, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@humancalico humancalico changed the title [WIP] add Deno.memoryInfo() (cli) add Deno.memoryInfo() Sep 5, 2020
@bartlomieju bartlomieju added this to the 1.4.0 milestone Sep 6, 2020
bartlomieju
bartlomieju requested changes Sep 8, 2020
View reviewed changes
cli/tests/unit/os_test.ts Outdated Show resolved Hide resolved
@humancalico @caspervonb
remove unecessary assertion
9c92d37 
Co-authored-by: Casper Beyer <caspervonb@pm.me>
@humancalico humancalico closed this Sep 8, 2020
@humancalico humancalico reopened this Sep 8, 2020
@bartlomieju bartlomieju mentioned this pull request Sep 9, 2020
Closed
@bartlomieju
Copy link
Member

Note to self, add @marcopacini as co-author when landing.

@bartlomieju bartlomieju changed the title (cli) add Deno.memoryInfo() feat(cli) add Deno.memoryInfo() Sep 9, 2020
piscisaureus
piscisaureus previously requested changes Sep 9, 2020
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

Documentation could use some love, otherwise LGTM for unstable.

We should carefully consider the name used before this becomes stable. This function returns 'system' memory info and not v8 heap info. Maybe call it systemMemoryInfo() ?

@humancalico
Copy link
Contributor Author

@piscisaureus Thanks for the review. I have added some documentation as per the man page of free and renamed the function from memoryInfo() to systemMemoryInfo(). Let me know what you think

@humancalico humancalico changed the title feat(cli) add Deno.memoryInfo() feat(cli) add Deno.systemMemoryInfo() Sep 10, 2020
bartlomieju
bartlomieju approved these changes Sep 10, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @humancalico

@bartlomieju bartlomieju merged commit c1b4ff6 into denoland:master Sep 10, 2020
@humancalico humancalico deleted the mem_info branch February 21, 2021 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caspervonb caspervonb caspervonb left review comments

@bartlomieju bartlomieju bartlomieju approved these changes

@piscisaureus piscisaureus piscisaureus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@humancalico @CLAassistant @bartlomieju @caspervonb @piscisaureus