[#39] Move to Ansifmt #46

chshersh · 2025-02-04T20:18:36Z

Closes #39

It was surprisingly easy to migrate! I haven't noticed any regressions in the behaviour. Label background and foreground based on luminance still work as expected:

I'm glad that quite a lot of bespoke logic got deleted, and there's a smaller context to keep in mind while working on this project.

Have a look and let me know if it makes sense!

github_tui.opam.template

chshersh · 2025-02-04T20:19:54Z

lib/pretty/color.ml

Glad this is gone! 🚀

chshersh · 2025-02-04T20:21:04Z

lib/pretty/pretty.mli

 (** A library for declarative UI definition. *)

+(** Common styles for GitHub TUI. *)
+module Style = Style


I'm no longer exporting Style from both Pretty.Doc and Pretty.Layout. It was an unfortunate coincidence anyway.

Now Style should be used as Pretty.Style

chshersh · 2025-02-04T20:22:15Z

lib/pretty/style.ml

+module Color = Ansifmt.Color
+module Styling = Ansifmt.Styling
+
+type t = Styling.t
+
+let ( & ) = Styling.( & )


I reexport things from Ansifmt to follow something kinda Domain-Driven Design and provide a small local wrapper around ansifmt so other modules inside GitHub TUI shouldn't depend on Ansifmt directly, only on relevant parts.

chshersh · 2025-02-04T20:23:03Z

lib/pretty/style.ml

+let none = Styling.none
+
+(* Repository name. *)
+let repo = Styling.(bold & fg Color.blue)


It would be nice to write just Styling.(bold & fg blue) without the Color but I don't really want to open Color.

Eh, fine for now, not a big deal.

Makes sense, perhaps we should re-export some of the common colors in Styling

Actually, I take it back. I'm doing the same Color.name in my website written in Elm:

https://github.com/chshersh/chshersh.github.io/blob/417529c96192109df6205f45d455687cf1ce5ac2/src/Page/Main.elm#L55

I guess, words like yellow or magenta have the only implicit meaning of color. But for more exotic colors like "suva grey" or "gainsboro" it's not that clear that it's a color. So having a Color. prefix is better.

I'll keep the prefix everywhere for now for consistency. Usually consistency is superior to typing fewer characters occasionally.

So I wouldn't push for reexporting colors. In GitHub TUI, I'll probably introduce some specific colors locally for different themes, so it's not a problem at all.

Fair enough, I was a bit skeptical anyway

chshersh · 2025-02-04T20:23:35Z

lib/pretty/style.ml

+let apply_background_hex_color ~hex text =
+  let color = Color.make_rgb_hex hex in
+  match color with
+  | None -> (text, `Light)
+  | Some color ->
+      let foreground_suggestion = Color.best_for_contrast ~threshold:80 color in
+      let contents = Printf.sprintf " %s " text in
+      let text = Styling.(wrap ~contents (bg color)) in
+      (text, foreground_suggestion)


This function was so ugly and unsafe 😭

I'm glad it's now much prettier and saner!

chshersh · 2025-02-04T20:24:29Z

test/test_pretty/test_pretty.ml

@@ -1 +1 @@
-let tests = List.flatten [ Test_color.tests ]
+let tests = List.flatten []


I was testing only my own Color implementation but now I don't have tests anymore, lol.

They'll be added later, don't worry. There're lots of tests to write.

chshersh · 2025-02-04T20:25:04Z

@qexat This is mostly for you to review. Feel free to share your feedback or any comments 🙂

qexat

Looks good to me. dune build @install builds fine locally.

Ansifmt 0.2.0 was just released, so we can pull it from the opam repository directly.

qexat · 2025-02-05T12:33:03Z

I made Ansifmt pulling its source from the Opam repository instead of from source, as 0.2.0 was just released: https://opam.ocaml.org/packages/ansifmt/

(The force-push was due to an incorrect formatting of the commit message 😅 I thought it would handle \n as a newline)

chshersh · 2025-02-05T13:06:59Z

I made Ansifmt pulling its source from the Opam repository instead of from source, as 0.2.0 was just released: https://opam.ocaml.org/packages/ansifmt/

(The force-push was due to an incorrect formatting of the commit message 😅 I thought it would handle \n as a newline)

Amazing! Thank you! 🙏🏻

heathhenley

🚀

heathhenley · 2025-02-05T14:19:38Z

dune build @install is failing for me but I'm probably doing something noobish - do you have do something else to sync your env?

qexat · 2025-02-05T14:58:04Z

@heathhenley it seems that you don't have ansifmt installed in your current switch

try running:

opam install . --deps-only --with-test

heathhenley · 2025-02-05T15:08:59Z

@heathhenley it seems that you don't have ansifmt installed in your current switch

try running:
opam install . --deps-only --with-test

🤦- thanks all good now!

Does anyone think we should throw a note in the readme about how to update switch when deps change / update?

I was also thinking about the list of 'non-obvious' things you mentioned in the other thread - wonder if using the log module to log debug info to a file would count (for dev)? And getting / setting your github token (for everyone)

chshersh · 2025-02-05T16:54:53Z

@heathhenley it seems that you don't have ansifmt installed in your current switch
try running:
opam install . --deps-only --with-test
🤦- thanks all good now!

Does anyone think we should throw a note in the readme about how to update switch when deps change / update?

I was also thinking about the list of 'non-obvious' things you mentioned in the other thread - wonder if using the log module to log debug info to a file would count (for dev)? And getting / setting your github token (for everyone)

I don't mind the idea for having tips for developers. Currently, we have a section in README for that. So feel free to add more to it, like updating a dependency or using logs 🙂

We don't have lots of docs yet, so it's fine to write everything in README for now. In the future, we'll probably have a separate docs/ directory with all the docs.

chshersh · 2025-02-05T16:55:16Z

Thanks @qexat and @heathhenley for the review! Merging 🙂

Move to Ansifmt

bc2c712

chshersh added component: UI Issues related to changing UI type: refactoring Improving the code quality w/o changing functionality labels Feb 4, 2025

chshersh self-assigned this Feb 4, 2025

chshersh commented Feb 4, 2025

View reviewed changes

github_tui.opam.template Outdated Show resolved Hide resolved

chshersh commented Feb 4, 2025

View reviewed changes

lib/pretty/color.ml

Copy link

Owner Author

chshersh Feb 4, 2025

Choose a reason for hiding this comment

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

Glad this is gone! 🚀

chshersh commented Feb 4, 2025

View reviewed changes

chshersh marked this pull request as ready for review February 4, 2025 20:24

chshersh requested review from heathhenley and qexat February 4, 2025 20:24

qexat approved these changes Feb 5, 2025

View reviewed changes

Make Ansifmt a normal dependency

c63639d

Ansifmt 0.2.0 was just released, so we can pull it from the opam repository directly.

qexat force-pushed the chshersh/ansifmt branch from dc3d9a2 to c63639d Compare February 5, 2025 12:32

qexat approved these changes Feb 5, 2025

View reviewed changes

heathhenley approved these changes Feb 5, 2025

View reviewed changes

chshersh merged commit deb91b8 into main Feb 5, 2025
3 checks passed

chshersh deleted the chshersh/ansifmt branch February 5, 2025 16:55

		@@ -1 +1 @@
		let tests = List.flatten [ Test_color.tests ]
		let tests = List.flatten []

Uh oh!

[#39] Move to Ansifmt #46

[#39] Move to Ansifmt #46

Uh oh!

Conversation

chshersh commented Feb 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chshersh commented Feb 4, 2025

Uh oh!

qexat left a comment

Choose a reason for hiding this comment

Uh oh!

qexat commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chshersh commented Feb 5, 2025

Uh oh!

heathhenley left a comment

Choose a reason for hiding this comment

Uh oh!

heathhenley commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qexat commented Feb 5, 2025

Uh oh!

heathhenley commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chshersh commented Feb 5, 2025

Uh oh!

chshersh commented Feb 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qexat commented Feb 5, 2025 •

edited

Loading

heathhenley commented Feb 5, 2025 •

edited

Loading

heathhenley commented Feb 5, 2025 •

edited

Loading