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

suggestion: restructure std/console, std/cli, std/fmt and std/text #4326

Closed
Leokuma opened this issue Feb 13, 2024 · 23 comments
Closed

suggestion: restructure std/console, std/cli, std/fmt and std/text #4326

Leokuma opened this issue Feb 13, 2024 · 23 comments
Labels
needs discussion Needs discussion this topic needs further discussion to determine what action to take.

Comments

@Leokuma
Copy link

Leokuma commented Feb 13, 2024

You guys probably have a vision for the future of those modules, but as they currently stand, I would like to propose these changes:

  1. ✅ Move std/console into std/cli and deprecate std/console
  2. Move fmt/colors to std/cli, and possibly rename from colors.ts to ansi.ts styles.ts as it also does bold and italic (I'm assuming the majority of people looking for ANSI color codes are targetting the terminal)
  3. Move fmt/printf/printf to std/cli (fmt/printf/sprintf would stay in fmt)
  4. ⬇️ Move text/case to std/fmt (see new idea below)
  5. Rename std/text to std/text_search. Not sure about this one. This would narrow down the module too much. Golang's String module has many functions that could be added to std/text in the future.

New idea for item 4: deprecate std/fmt entirely:

  • Move std/fmt/bytes.ts to std/bytes
    • Questionable because bytes.ts formats for example 1337 → 1.34 kB, so it handles Number, not Uint8Array or ArrayBuffer. It might make more sense to have such a function in an std/number, but there's no std/number.
  • Move std/fmt/colors.ts to std/cli (item 2)
  • Move std/fmt/duration.ts to std/datetime
    • Maybe not worth the breaking change because both std/fmt/duration.ts and std/datetime might be deprecated once Temporal is stabilized
  • Move std/fmt/printf.ts to std/text (impacts item 3)
@Leokuma Leokuma changed the title Suggestion: restructure std/console, std/cli, std/fmt and std/text suggestion: restructure std/console, std/cli, std/fmt and std/text Feb 13, 2024
@iuioiua
Copy link
Contributor

iuioiua commented Feb 14, 2024

I agree with 1-3, am unsure about 4, and disagree with 5, as std/text_search narrows down the purpose of that module too much.

@iuioiua iuioiua added the needs discussion Needs discussion this topic needs further discussion to determine what action to take. label Mar 28, 2024
@iuioiua
Copy link
Contributor

iuioiua commented Apr 4, 2024

I talked with Yoshiya, and we're in favour of merging std/console into std/cli and deprecating std/console. We'll make sure to get back to you about the other points.

@iuioiua
Copy link
Contributor

iuioiua commented Apr 15, 2024

I'm in favour of moving fmt/colors to std/cli, as it's for colours in the terminal. WDYT, @kt3k?

@Leokuma
Copy link
Author

Leokuma commented Apr 15, 2024

I've updated item 4 with a new suggestion to deprecate std/fmt.

@iuioiua
Copy link
Contributor

iuioiua commented Apr 16, 2024

The new suggestions make a lot of sense to me. However, the printf() makes it seem like std/fmt/printf should be moved to std/cli instead of std/text. I think printf() is redundant, as you can just use sprintf() with console.log() and it achieves the same effect.

I suggest deprecating printf() in favour of using sprintf() with console.log(). That way, we can move std/fmt/printf to std/text/sprintf. @kt3k, WDYT?

@kt3k
Copy link
Member

kt3k commented Apr 16, 2024

printf just sounds familiar and looks good to keep. Only providing sprintf without providing printf feels strange to me.

@iuioiua
Copy link
Contributor

iuioiua commented Apr 16, 2024

Alternatively, we can have printf() use console.log() instead of Deno.stdout for greater compatibility with other runtimes. @kt3k, WDYT about the other 3 suggestions? I like them.

@kt3k
Copy link
Member

kt3k commented Apr 16, 2024

Alternatively, we can have printf() use console.log() instead of Deno.stdout for greater compatibility with other runtimes.

I don't think it's good idea to use console.log for printf as it can't print \n by default. I think we should use process.stdout.write if we try to make it node compatible.

@kt3k
Copy link
Member

kt3k commented Apr 16, 2024

Move std/fmt/bytes.ts to std/bytes

Not sure about this, but it feels a bit confusing to me. The APIs in std/bytes operate with Uint8Arrays, but std/fmt/bytes works with byte numbers, not byte data itself.

Move std/fmt/colors.ts to std/cli (item 2)

I'm not sure about this. We need more feedback from the community and core team.

Move std/fmt/duration.ts to std/datetime

Doesn't sound a good idea to me as it's highly likely that we deprecate std/datetime entirely after Temporal got stable.

@Leokuma
Copy link
Author

Leokuma commented Apr 16, 2024

Move std/fmt/bytes.ts to std/bytes

it feels a bit confusing to me. The APIs in std/bytes operate with Uint8Arrays, but std/fmt/bytes works with byte numbers, not byte data itself.

True. I was misled by the names. It might make more sense to move bytes.ts (then renamed to byte_format.ts or something) to std/number, which doesn't exist. Now is it worth creating a std/number? Go doesn't have a number or integer module. Lodash does have a number module with 3 functions: clamp, inRange and random.

Move std/fmt/duration.ts to std/datetime

Doesn't sound a good idea to me as it's highly likely that we deprecate std/datetime entirely after Temporal got stable.

We could move it so that it's easier to deprecate everything together in the future because duration.ts will probably be deprecated too in favor of Temporal.Duration.toLocaleString().

I'm still inclined to pursue the deprecation of fmt in favor of moving formatting functions to the modules that handle the specific datatype (number, date, string, JSON etc). I don't really know Go, but I'm guessing Go's fmt is mostly about manipulating mutable strings and byte sequences in a memory-efficient way (which is not trivial in compiled, low level languages), like Java's StringBuilder. It doesn't translate so directly to the JS world. Most of those functions would fit well into std/text, while others could be ported to std/cli and std/bytes.

@iuioiua
Copy link
Contributor

iuioiua commented Apr 29, 2024

I want to focus on std/fmt/colors.ts. Its renaming to std/cli/colors.ts is misleading because the module takes care of more than colors. For example, it has bold() and italic(), which are not colors. I suggest we move it to std/cli/ansi.ts. Moving to std/cli makes a lot sense to me, as these functions are entirely oriented to terminal output.

@kt3k
Copy link
Member

kt3k commented Apr 29, 2024

Google Chrome supports ANSI escape code in the console.

Screenshot 2024-04-29 at 14 02 23

Maybe this is not specific to terminal

@iuioiua
Copy link
Contributor

iuioiua commented Apr 29, 2024

I use the words terminal and console interchangeably (perhaps, I shouldn't). Even still, that's more to my point of moving it to std/cli.

@Leokuma
Copy link
Author

Leokuma commented Apr 29, 2024

I agree with moving and renaming to std/cli/ansi.ts.

Months ago I was looking for a function to add colors to the terminal. I looked first in cli and then in console. I couldn't find anything and thought std didn't have it. I was surprised to find it in fmt.

@kt3k
Copy link
Member

kt3k commented Apr 30, 2024

Months ago I was looking for a function to add colors to the terminal. I looked first in cli and then in console. I couldn't find anything and thought std didn't have it. I was surprised to find it in fmt.

I don't see strong enough motivation for moving it. fmt/colors are largely accepted/used by the community for years (See https://github.com/search?q=fmt%2Fcolors.ts+language%3ATypeScript&type=code&l=TypeScript ) , and this is the first report of that kind.

@iuioiua
Copy link
Contributor

iuioiua commented Apr 30, 2024

Moving std/fmt/colors.ts to std/cli/ansi.ts is worth the breaking change at v1 because:

  1. The module is solely oriented for the console. std/fmt is not an obvious location for the module.
  2. Having a filename, colors.ts is misleading because it does much more than that. E.g., italic and bold styling.

Having it be std/fmt/colors.ts makes no sense.

@kt3k
Copy link
Member

kt3k commented Apr 30, 2024

I wonder if ansi is a good name for it. ANSI escape code is not the only spec from ANSI org. What names do other langauges' standard libraries use?

@kt3k
Copy link
Member

kt3k commented Apr 30, 2024

Having it be std/fmt/colors.ts makes no sense.

It's widely accepted in the Deno ecosystem ( https://github.com/search?q=fmt%2Fcolors.ts+language%3ATypeScript&type=code&l=TypeScript ) and that argument ('fmt/colors is confusing') is quite new.

Also this issue doesn't seem getting enough attention from the community.

@Leokuma
Copy link
Author

Leokuma commented Apr 30, 2024

fmt/colors are largely accepted/used by the community for years (See https://github.com/search?q=fmt%2Fcolors.ts+language%3ATypeScript&type=code&l=TypeScript ), and this is the first report of that kind.

That's probably because std/cli and std/console didn't exist before. Once we created them, std/fmt/colors looked out of place.

I understand that breaking changes like that always bring some nuisance, but in this case I think it makes so much sense users will understand and not be so bothered. Maybe they even like it because they'll be able to import everything from cli instead of importing a bit from console, a bit from fmt and a bit from cli.

@kt3k
Copy link
Member

kt3k commented May 1, 2024

That's probably because std/cli and std/console didn't exist before. Once we created them, std/fmt/colors looked out of place.

Sounds a fair point.

@iuioiua
Copy link
Contributor

iuioiua commented May 3, 2024

I wonder if ansi is a good name for it. ANSI escape code is not the only spec from ANSI org. What names do other langauges' standard libraries use?

On second thought, using "ansi" in the filename may have two negative effects:

  1. To beginners, it might not be recognizable.
  2. To those who know ANSI escape codes, it might be misleading, possibly implying that the module also implements control sequences such as cursor movement.

std/cli/styles.ts might be better.

@Leokuma
Copy link
Author

Leokuma commented May 3, 2024

Agreed. "Styles" is more specific and descriptive of what it does.

@Leokuma
Copy link
Author

Leokuma commented Jul 8, 2024

Closing this issue so that we can discuss each remaining point in separate, more specific issues.

@Leokuma Leokuma closed this as completed Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Needs discussion this topic needs further discussion to determine what action to take.
Projects
None yet
Development

No branches or pull requests

3 participants