Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(console): support dynamic style injections #703

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

innocenzi
Copy link
Member

@innocenzi innocenzi commented Nov 8, 2024

This pull request adds support for dynamic style injections, responding to the need to write arbitrary styles.

For context, I have this need in my upcoming "prompts" pull request—I am currently adding injections as I see the need for them, but I need too many colors (eg. danger, warning, hint, dim...) which don't really fit the semantic injection paradigm we have now.

Essentially, the following is now possible:

$this->console->write('<style="fg-cyan bg-white underline">Tempest</style>');
Before edit
// foreground color
$this->console->write('<fg=cyan>Tempest</fg>');
$this->console->write('<fg=darkcyan>Tempest</fg>');

// background color
$this->console->write('<bg=cyan>Tempest</bg>');
$this->console->write('<bg=darkcyan>Tempest</bg>');

// modifiers
$this->console->write('<mod=underline>Tempest</mod>');
$this->console->write('<mod=bold>Tempest</mod>');

This works by adding a new DynamicInjection injection, which will parse the tag and styles and will look for a corresponding values in the TerminalStyle enum from tempest/highlight.

See also: tempestphp/highlight#162

@innocenzi innocenzi requested a review from brendt November 8, 2024 19:09
@brendt
Copy link
Member

brendt commented Nov 9, 2024

I think it's a good feature, but I always disliked Symfony's style tags (these look very similar). Couldn't we do something like

<style="fg-cyan bg-red underline">…</style>

Or something like

<fg-cyan bg-red underline>…</>

IDK about the syntax, but I'd like to brainstorm some more about it first.

@innocenzi
Copy link
Member Author

Fair enough, here are my thoughts:

  • The advantage of <fg>, <bg> and <mod> is that it's easy to implement with regex and concise, but you need multiple tags to apply multiple styles at once
  • The advantage of <style> is that you can have multiple styles at once, but the tag itself is longer
  • I personally don't really like <fg-cyan bg-red> because it would be harder to parse and it's an unusual syntax

Out of those three, I don't have a strong opinion, I like both the <fg> one and <style> one

@aidan-casey any opinion here?

@innocenzi
Copy link
Member Author

I can think of two more syntaxes:

  • <text fg="cyan" bg="red" mod="underline">Tempest</text>
    Similar to style, but with explicit key/values instead of a list of strings? Might be even harder to parse though.

  • @bg-dark-red @fg-white @underline Tempest @reset
    A token-based approach, vastly different than the tag-based one. Super easy to parse, very flexible for the user, but a slightly worse readability. I think the readability tradeoff might be acceptable because it's a syntax that would not often be needed.

We can use something else than @ too, and allow escaping.

@brendt
Copy link
Member

brendt commented Nov 12, 2024

A token-based approach, vastly different than the tag-based one. Super easy to parse, very flexible for the user, but a slightly worse readability. I think the readability tradeoff might be acceptable because it's a syntax that would not often be needed.

There might be something to this. I do think with need something else than the @ approach, because of whitespacing, that'll get confusing very quickly.

{bg-dark-red}{fg-white}{underline}Tempest{reset}

But writing it like this reminds me that it's essentially the same thing Symfony does…

<info>Tempest</>

I was reminded of how tempest/highlight works:

{:bg-dark-red:fg-white:underline:Tempest:}

But that's more difficult to parse again, and not really readable…

That makes me circle back to what you originally implemented, or maybe the style tag. I like the idea of being able to combine multiple styles in one tag, I think that's slightly more convenient. It's definitely something I missed when using Symfony's console styling

@innocenzi
Copy link
Member Author

Let's go with the style tag then 👍

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11799352273

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 44 of 51 (86.27%) changed or added relevant lines in 4 files are covered.
  • 28 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.03%) to 82.523%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Tempest/Console/src/Highlight/TempestConsoleLanguage/Injections/DynamicInjection.php 29 31 93.55%
src/Tempest/Console/src/Highlight/DynamicTokenType.php 12 17 70.59%
Files with Coverage Reduction New Missed Lines %
src/Tempest/Http/src/Commands/ServeCommand.php 1 0.0%
src/Tempest/Console/src/ConsoleApplication.php 7 10.0%
src/Tempest/Support/src/StringHelper.php 8 94.51%
src/Tempest/Http/src/HttpApplication.php 12 0.0%
Totals Coverage Status
Change from base Build 11751823795: 0.03%
Covered Lines: 7333
Relevant Lines: 8886

💛 - Coveralls

@brendt
Copy link
Member

brendt commented Nov 13, 2024

Cool! Is this ready for a final review then?

@innocenzi
Copy link
Member Author

@brendt yes it is!

@brendt
Copy link
Member

brendt commented Nov 13, 2024

This is so great, I've got nothing to add!

Oh maybe one more thing after merging: did you intend to write docs for this, or do you want me to tackle that?

@brendt brendt merged commit 6847a79 into tempestphp:main Nov 13, 2024
58 checks passed
@innocenzi innocenzi deleted the feat/console/dynamic-injection branch November 13, 2024 18:50
@innocenzi
Copy link
Member Author

To be honest I initially thought of this as a kind of internal feature, but now that I think more about it, you're right—it could totally be documented

If you want to tackle it, feel free, otherwise I'll do it with my console components PR :)

@brendt
Copy link
Member

brendt commented Nov 14, 2024

I've made an issue for it, we'll see who gets to it first

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants