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

proposal: Move std/flags to std/cli #3526

Closed
lino-levan opened this issue Aug 8, 2023 · 11 comments · Fixed by #3530
Closed

proposal: Move std/flags to std/cli #3526

lino-levan opened this issue Aug 8, 2023 · 11 comments · Fixed by #3530

Comments

@lino-levan
Copy link
Contributor

lino-levan commented Aug 8, 2023

Currently, we have this top level module which is both confusingly named (what is flags referring to? -- not immediately clear imo) and incredibly limiting in scope. I think this would be a lot better if the TLM was named std/cli. This would also open a lot of opportunities for new utilities that I would find quite useful imo.

See #3488 for context on initial discussion.

cc @jeff-hykin @kt3k @iuioiua

@jeff-hykin
Copy link
Contributor

This would also provide a nice space for other CLI tools such as helpers for consuming stdin, common prompts such as "askForInt", a standard way of detecting terminal color support, etc.

@kt3k
Copy link
Member

kt3k commented Aug 11, 2023

I'm a little against this direction because such kind of features tend to belong to 'CLI framework' categories (They tend to need to be opinionated in some way).

Also there are at least 2 major frameworks (yargs, cliffy) existing which officially support Deno. I'm not sure it's good idea to start competing with them at this point.

a standard way of detecting terminal color support, etc.

FYI better color support is being done in denoland/deno#18549

@jeff-hykin
Copy link
Contributor

jeff-hykin commented Aug 13, 2023

FYI better color support is being done in denoland/deno#18549

Cool! I do question it being in core though as

  1. It would nice to have detection on all versions of Deno, not just the latest
  2. Color support isn't a exact science but more of detecting many pseudo-standards based of env vars and OS. New terminals often means new pseudo-standards, which to me feels more appropriate for a module (easy to update/backport and fork) rather than a built-in. For example if term==dumb and TRUECOLOR=true, those are contradictory pieces of information and there's not really a "right" way to resolve them.

They tend to need to be opinionated in some way

I totally agree the std should be unopinionated, and that's exactly what is different about these CLI tools. Because of that I don't think they will compete with cliffy and the like. For example the askFor.int is going to be very close to the builtin prompt+validation and be completely different from cliffy's colorful, unstable-required-for-older-deno, chainable, int-range input. I like cliffy, and people still have every reason to keep using cliffy.

But for most of my cli's I prefer Deno flags because it's a tool not a framework, it's mostly unoptinonated, and it's a minimal + flat dependency that's vetted and stable instead of a giant chain of deps crammed with features. If anything I would actually like to flags to be more unopionated (e.g. make it so that --_ doesn't break things, allow repeating flags, get the index of args/flags, etc)

Unlike cliffy or args, every tool under cli/ should work independently, and I think didYouMean and flags are a good examples.

@jsejcksn
Copy link
Contributor

jsejcksn commented Aug 19, 2023

I support a change that renames flags to some form of the proper term arguments (e.g. args, parse_args, etc.).

@jeff-hykin
Copy link
Contributor

I support a change that renames flags to some form of the proper term arguments (e.g. args, parse_args, etc.).

I literally have a vs code snipped called parseArgs that expands to import the Deno flags haha

@iuioiua
Copy link
Contributor

iuioiua commented Nov 6, 2023

I'm weakly against this but need a clearer image. What do we think the list of symbols would be for an std/cli sub-module? I'll start.

  • parseFlags() or parseArgs()

@jeff-hykin
Copy link
Contributor

jeff-hykin commented Nov 7, 2023

Before I list some things, I want to mention I make bootstrapping scripts where audit-ability, correctness and simplicity are key.

Think of these tools as things that could go in the deno bootstrapping script

  • promptPassword (we do NOT want some 3rd party handling that kind of logic)
  • Console rows/columns would be in this list except its built all the way into deno core 🎉
  • Console color support detection
  • Having a tool for updating a line, e.g. Deno.stdout.write(new TextEncoder().encode(something+"\r"))
  • A tool that reads/consumes all the stdin as a string instead of having to write the iteration, aggregation, and string conversion manually
  • some basic input validation; promptYesNo, promptNumber. Nothing fancy, no colors, no multi-select, checkboxes or chaining. Just barebones cli data extraction.
  • Have some kind of hook for "onConsoleSizeChange"
  • I would still like a "didYouMean" helper, but theres a separate thread for that conversation

@iuioiua
Copy link
Contributor

iuioiua commented Nov 7, 2023

I'll preface the following replies by saying there's no need to fixate on these technicalities. These are just initial thoughts on ideas. Again, the aim is to picture what a std/cli might look like and then decide whether it's worth having.


  • promptPassword (we do NOT want some 3rd party handling that kind of logic)

+1 for something like this. My idea was promptSecret(). To me, this is the most obvious feature to add.

  • Cosole rows/columns would be in this list except its built all the way into deno core 🎉

Sorry, what would it do?

  • Console color support detection

What would be a use case?

  • some basic input validation; promptYesNo, promptNumber. Nothing fancy, no colors, no multi-select, checkboxes or chaining. Just barebones cli data extraction.

Thing is, some of these are so trivial to do by yourself that I wonder whether it'd make sense to have them in the Standard Library. Also, IIUC promptYesNo() would overlap too much with confirm().

  • I would still like a "didYouMean" helper, but theres a separate thread for that conversation

Yep, that's #3752.

@jeff-hykin
Copy link
Contributor

I probably shouldve mentioned this first; even if parseFlags is the ONLY tool, I still think it would still be good to have it under std/cli.

std/flags just isn't good communication. It's not a category like "async" or "file system". I only found parseArgs after brute-force looking at every single thing in std (regardless of name).

  • Console rows/columns would be in this list except its built all the way into deno core 🎉

Sorry, what would it do?

Sorry, you can ignore that comment. Back in older versions like Deno 1.25 I would've said "Deno std needs a helper for getting the width and height of the user's terminal" but recent versions of Deno actually just have it as a built-in feature.

Also, IIUC promptYesNo() would overlap too much with confirm().

Oh! I didn't realize Deno made use of that API. I definitely agree that would be redundant.

  • Console color support detection

What would be a use case?

Knowing if color would be rendered correctly? Even if Deno.noColor is false, it doesn't mean the users terminal actually supports color output.

some of these are so trivial to do by yourself

I apologize if this comes off as harsh, don't mean it personally, it's just this^ argument really grinds my gears:

You could say that about half the stuff in std. Look at deferedPromise, weekOfYear, or stuff like assertExists which is literally ONE if statement.

More importantly, look at confirm() itself

  • Built-in things have a cost (bloating the users runtime even if they never use it). But std doesn't have that cost.
  • Despite that cost, a function more trivial than parseInt, was deemed important enough to be built-in to browsers.

Triviality has nothing to do with whether or not people would find a function useful.

If std is not designed to reduce code duplication, then what is its purpose?

@iuioiua
Copy link
Contributor

iuioiua commented Nov 8, 2023

Fair points, @jeff-hykin. I now agree that we should have an std/cli sub-module with parseFlags() (or similar) and promptSecret(). IMO, just those are good enough to have the sub-module.

However, I think the sub-module must remain plain and unopinionated and doesn't do more than it needs to.

WDYT, @kt3k?

@iuioiua
Copy link
Contributor

iuioiua commented Nov 9, 2023

Alrighty, let's go ahead with this move. @lino-levan, I've done an initial review of your PR.

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 a pull request may close this issue.

5 participants