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

Inconsistency in Styling API #135

Open
SirNickolas opened this issue Dec 1, 2023 · 2 comments
Open

Inconsistency in Styling API #135

SirNickolas opened this issue Dec 1, 2023 · 2 comments

Comments

@SirNickolas
Copy link
Contributor

auto myBold = bold;
assert(is(typeof(myBold("text")) == string));
assert(!is(typeof(bold("text")): string)); // Not even convertible.

(That’s because bold()("text") returns string while bold("text") returns StyledText.)

IMHO, it shouldn’t work like this. Unfortunately, any change in this would be an API change. Is it justified for 2.0?

In case we decide to unify them, we’ll have at least 3 ways:

  1. Make everything return string, remove StyledText altogether.
  2. Make everything return StyledText, which will be implicitly convertible to string.
  3. Make everything return StyledText, which will retain its explicit toString method.

Why do we consider options 2 and 3? Because StyledText can become a lazy range – and it’ll be possible to rule out allocations of temporary strings:

// Allocates a temporary just to concatenate it; then it immediately becomes garbage.
string msg0 = "argument " ~ bold("name") ~ " is bold";

// Proposed alternative:
auto app = appender!string();
app ~= "argument ";
app ~= bold("name"); // No temporaries; writes directly into the buffer.
app ~= " is bold";
string msg1 = app[];

// The same in one line:
string msg2 = chain("argument ".byCodeUnit, bold("name"), " is bold".byCodeUnit).array();

I think StyledText can retain its opBinary!"~" and opBinaryRight!"~" for convenience: they clearly look like something that allocates. So all the snippets above will be compilable.

The question is, whether or not StyledText should implicitly convert to string. To be honest, a lazy range that can implicitly lose its laziness scares me off. I’d prefer option 3.

Implications:

  1. string msg = bold("text").toString() will continue to work.
  2. app ~= config.programName("text") will continue to work.
  3. writeln(config.programName("text")) will continue to work: writeln invokes toString.
  4. string msg = config.programName("text") ~ " is bold" will continue to work.
  5. string msg = config.programName("text") will break with approach 3: an explicit toString is required, just like in example 1 above.
@andrey-zherikov
Copy link
Owner

I tried to optimize generation of styled text (to postpone adding ANSI sequences until toString). Maybe it was over engineering. I'll try to make it simpler.

@andrey-zherikov
Copy link
Owner

5. string msg = config.programName("text") will break with approach 3: an explicit toString is required, just like in example 1 above.

Why are you saying that it "will break"? It doesn't compile even now so I see no problem here:

Error: cannot implicitly convert expression StyleImpl("text") of type StyledText to string

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

No branches or pull requests

2 participants