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

added osType() function #5714

Merged
merged 9 commits into from
Jun 11, 2020
Merged

added osType() function #5714

merged 9 commits into from
Jun 11, 2020

Conversation

rubiin
Copy link
Contributor

@rubiin rubiin commented May 21, 2020

No description provided.

@afinch7
Copy link
Contributor

afinch7 commented May 24, 2020

You should be able to get tests to pass by updating the APIs not yet implemented test for std/node. Here's a patch:

diff --git a/std/node/os_test.ts b/std/node/os_test.ts
index f0b9ca79..c67879d2 100644
--- a/std/node/os_test.ts
+++ b/std/node/os_test.ts
@@ -244,13 +244,6 @@ test({
       Error,
       "Not implemented"
     );
-    assertThrows(
-      () => {
-        os.type();
-      },
-      Error,
-      "Not implemented"
-    );
     assertThrows(
       () => {
         os.uptime();

ry
ry reviewed May 24, 2020
View reviewed changes
cli/js/lib.deno.ns.d.ts Outdated
@@ -155,6 +155,8 @@ declare namespace Deno {
*/
export function execPath(): string;

export function osType(): string;
Copy link
Member

Choose a reason for hiding this comment

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

Hi - thanks for the patch - but in order to add new APIs they need to first be unstable. Please move this declaration to cli/js/lib.deno.unstable.d.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ry
ry reviewed May 24, 2020
View reviewed changes
cli/ops/os.rs Outdated
_zero_copy: Option<ZeroCopyBuf>,
) -> Result<JsonOp, OpError> {
state.check_env()?;
let os_type = sys_info::os_type().unwrap_or_else(|_| "".to_string());
Copy link
Member

Choose a reason for hiding this comment

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

Add a check_unstable call here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ry made the changes

@rubiin rubiin requested a review from ry May 24, 2020 18:17
@rubiin
Copy link
Contributor Author

rubiin commented Jun 4, 2020

@ry any updates on this?

Alhadis
Alhadis suggested changes Jun 7, 2020
View reviewed changes
std/node/os.ts
@@ -190,9 +190,9 @@ export function totalmem(): number {
notImplemented(SEE_GITHUB_ISSUE);
}

/** Not yet implemented */
/** Returns the operating system name as returned by uname(3) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:
If this function returns the host's uname(3), then it might be better to call it osName() instead of osType(). The latter could be taken differently; i.e., referring to the operating system family (POSIX, Windows, Android, iOS, etc). At least, that was my immediate interpretation of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AleksandrukTad changed them as you asked

@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS labels Jun 9, 2020
@bartlomieju bartlomieju added this to the 1.1.0 milestone Jun 9, 2020
ry
ry approved these changes Jun 11, 2020
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @rubiin - sorry for the delay!

@ry ry merged commit 6ccf903 into denoland:master Jun 11, 2020
@rubiin
Copy link
Contributor Author

rubiin commented Jun 11, 2020

LGTM - thanks @rubiin - sorry for the delay!

No biggie :)

@ry
Copy link
Member

ry commented Jun 11, 2020

@rubiin Something just occurred to me - how is this different than Deno.build.os?

@KluVerKamp
Copy link
Contributor

I got this:

deno eval -p "Deno.build.os"
darwin

vs

deno eval -p "Deno.osName()" --unstable
Darwin

ry added a commit to ry/deno that referenced this pull request Jun 12, 2020
@ry
Revert "feat: add Deno.osName() (denoland#5714)"
d3abbc1 
Deno.build.os provides the same functionality

This reverts commit 6ccf903.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
@rubiin
feat: add Deno.osName() (denoland/deno#5714)
fb91719
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@rubiin
feat: add Deno.osName() (denoland/deno#5714)
5d63a91
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@rubiin @caspervonb
feat: add Deno.osName() (denoland/deno#5714)
9551ff9
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@rubiin @caspervonb
feat: add Deno.osName() (denoland/deno#5714)
377732f
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@rubiin @caspervonb
feat: add Deno.osName() (denoland/deno#5714)
cb0d9d5
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@rubiin @caspervonb
feat: add Deno.osName() (denoland/deno#5714)
d93cf0d
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@rubiin @caspervonb
feat: add Deno.osName() (denoland/deno#5714)
e7d3e7e
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@rubiin @denobot
feat: add Deno.osName() (denoland/deno#5714)
7c4033e
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
@rubiin @denobot
feat: add Deno.osName() (denoland/deno#5714)
054437b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alhadis Alhadis Alhadis requested changes

@ry ry ry approved these changes

Assignees
No one assigned
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
6 participants
@rubiin @afinch7 @ry @KluVerKamp @Alhadis @bartlomieju