Skip to content

Add various APIs#213

Merged
fpseverino merged 22 commits intovapor:consolekit-5from
fpseverino:update
Jul 2, 2025
Merged

Add various APIs#213
fpseverino merged 22 commits intovapor:consolekit-5from
fpseverino:update

Conversation

@fpseverino
Copy link
Copy Markdown
Member

  • Add a lot of various APIs (mainly APIs that Vapor Toolbox would need)
  • Add a lot of tests

@fpseverino fpseverino requested review from MahdiBM and ptoffy June 1, 2025 23:12
@fpseverino fpseverino requested review from 0xTim and gwynne as code owners June 1, 2025 23:12
Copy link
Copy Markdown
Member

@ptoffy ptoffy 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 also add test coverage

/// - Parameter style: The ``ConsoleStyle`` to use for styling.
///
/// - Returns: The string wrapped in ANSI codes.
public func stylized(_ style: ConsoleStyle) -> String {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bad idea, we should not extend standard types with public API as it pollutes all of the {Sub}String namespace. String doesn't care about this.
Also it's just way too general of a method (name) to add and it could likely conflict with any string manipulation library, even just with an internal stylized method

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In my opinion this is a very useful API, which cannot be recreated by end users as it uses private methods.
It comes in very handy when you want to integrate ConsoleKit with swift-argument-parser, as the latter has APIs that accept simple strings (obviously), but could benefit from ANSI stylisation.

Maybe if I change the function name to emphasise that it's from ConsoleKit is better?
Perhaps something like terminalStylized() or consoleStylized()?

Copy link
Copy Markdown
Member

@ptoffy ptoffy Jun 3, 2025

Choose a reason for hiding this comment

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

Personally I'd be happier with something like

let stylizedString = ConsoleStyle.style(myString, with: myStyle)

but happy to let others chime in for other ideas (or to prove me wrong 🙂)
(Having something like consoleStylized() is definitely more defensible than styled)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I disagree - I think this is fine if given a less generic name.

/// - isBold: If `true`, the text will be bold.
///
/// - Returns: The string wrapped in ANSI codes.
public func stylized(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above. I'd make it internal

@fpseverino fpseverino requested a review from ptoffy June 3, 2025 13:02
@fpseverino
Copy link
Copy Markdown
Member Author

We should also add test coverage

I think it's enabled only for the main branch(?)
Is there a way to reuse our workflow on another branch?

@fpseverino fpseverino merged commit ca482c4 into vapor:consolekit-5 Jul 2, 2025
12 checks passed
@fpseverino fpseverino deleted the update branch July 2, 2025 16:21
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.

4 participants