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: Stabilize Deno.consoleSize() API #15933

Merged
merged 14 commits into from Oct 25, 2022

Conversation

bartlomieju
Copy link
Member

Closes #15929

@bartlomieju bartlomieju added this to the 1.26 milestone Sep 17, 2022
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM. Just a note: this op can be a fast call, if the ConsoleSize result is put into a Uint32Array buffer.

@lucacasonato
Copy link
Member

I am not in favor of stabilizing as is, because it takes in a rid parameter. Should be Deno.stdin.consoleSize. See also the stabilization issue for Deno.setRaw.

@bartlomieju
Copy link
Member Author

@littledivy good point, I'll update the PR.

@lucacasonato also a good point. I'll rework it.

@bartlomieju
Copy link
Member Author

@lucacasonato one question though: should we make it Deno.stdin.consoleSize()? Seems a bit strange to get a size of console from stdin, maybe it should be a method on Deno.stdout and Deno.stderr?

@lucacasonato
Copy link
Member

Yeah sorry, that makes a lot more sense indeed.

@dsherret
Copy link
Member

dsherret commented Sep 21, 2022

maybe it should be a method on Deno.stdout and Deno.stderr

What if both stdout and stderr are piped, but you still want to get the console size? I think this should just be on the Deno namespace, though it also begs the question of if this should be providing a rid of stdout and stderr (https://github.com/denoland/deno/pull/15933/files#r976653975).

runtime/ops/tty.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

We should think more about how this would work with a pty before stabilizing (also, not done yet, but we shouldn't move this method to stdout and stderr). Additionally, this function should probably just work without providing an rid, then you provide the rid of a pty to get its size.

@lucacasonato
Copy link
Member

In that case, does it even need a rid parameter? Can it just be Deno.consoleSize() with no args?

@dsherret
Copy link
Member

dsherret commented Sep 21, 2022

In that case, does it even need a rid parameter? Can it just be Deno.consoleSize() with no args?

Yes, I think that would be best.

Also, related to the other Deno.stdin.setRaw, we could just have a pty.consoleSize() or something for ptys.

@shadowspawn
Copy link

I'm not sure it makes sense to provide stdout and stderr here because if those are piped there doesn't seem to be a way to get the console screen size.

If they are both piped, is there even a console? I might have the concept wrong!

The use case I came looking for consoleSize was for finding the word-wrap width for displaying help text from a command-line utility. The help is being displayed to either stdout or stderr, and I was expecting to find the width of the one being used, which might be a pipe (and hence width not known).

@dsherret
Copy link
Member

If they are both piped, is there even a console?

@shadowspawn huh, I thought there was some low level api to still get it based on the parent process (or console group or whatever it's called) because I had seen api's like let size = terminal_size()? or in the case of .net var width = Console.WindowWidth;, but looking at all those implementations I found they're all providing stdout's handle. Additionally, it seems it's possible to get the console size via stdin (https://stackoverflow.com/a/59717851/188246).

That said, I still think we should consider just making this Deno.consoleSize() and perhaps have it try to get the size via stdout, stderr, then stdin, and if not then throw. Otherwise, then perhaps it should be on all three (Deno.stdout.consoleSize(), Deno.stdin...., Deno.stdin....) if that indeed works on stdin. Making it Deno.consoleSize() seems easiest for users, but maybe it's not versitle enough?

The use case I came looking for consoleSize was for finding the word-wrap width for displaying help text from a command-line utility. The help is being displayed to either stdout or stderr, and I was expecting to find the width of the one being used, which might be a pipe (and hence width not known).

Perhaps in a future api (with isatty moved to Deno.stdout.isatty()) the code would do:

const size = Deno.stdout.isatty() ? Deno.consoleSize() : undefined;

@bartlomieju
Copy link
Member Author

I changed the API to not accept any arguments. It will use stdin handle to get the size of the console. @dsherret @lucacasonato please take a look

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM, but there is an std submodule update that I think should be reverted?

@bartlomieju
Copy link
Member Author

LGTM, but there is an std submodule update that I think should be reverted?

Not really, after merging this PR I will update deno_std in accordance with this API and then update it again in CLI

@dsherret
Copy link
Member

Oh, I see.

@bartlomieju bartlomieju merged commit ab0c33e into denoland:main Oct 25, 2022
@bartlomieju bartlomieju deleted the stabilize_deno_consolesize branch October 25, 2022 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stabilize Deno.consoleSize()
5 participants