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

colors start at beginning of line instead of beginning of element #44

Closed
lread opened this issue Sep 28, 2019 · 4 comments · Fixed by #46
Closed

colors start at beginning of line instead of beginning of element #44

lread opened this issue Sep 28, 2019 · 4 comments · Fixed by #46
Labels

Comments

@lread
Copy link
Contributor

lread commented Sep 28, 2019

Hello! First, thanks so much for puget, it is a wonderful library!

While experimenting with using background colors with puget, I noticed an interesting behavior.

If I cprint with the default :color-scheme,

(require '[puget.printer :as puget])

(def t [nil true \space "string"
        {:omega 123N :alpha '(func x y) :gamma 3.14159}
        #{\a "heterogeneous" :set}
        (java.util.Currency/getInstance "USD")
        (java.util.Date.)
        (java.util.UUID/randomUUID)])

(puget/cprint t)

I get similar results to the README screenshot:
image

But if I try background colors:

(defn bg-cprint [ expr ]
  (puget/cprint expr
                {:color-scheme
                 {:delimiter [:black :bold :bg-red]
                  :tag       [:black :bold :bg-red]
                  :nil       [:white :bold :bg-black]
                  :boolean   [:black :bg-green]
                  :number    [:black :bg-cyan]
                  :string    [:black :bold :bg-magenta]
                  :character [:black :bold :bg-magenta]
                  :keyword   [:black :bold :bg-yellow]
                  :symbol    nil
                  :function-symbol [:black :bold :bg-blue]
                  :class-delimiter [:black :bg-blue]
                  :class-name      [:black :bold :bg-blue]}}))
(bg-cprint t)

notice that the color starts at the beginning of the line rather than the beginning of the element:
image

A nested expression:

(bg-cprint ["item1"
            ["item2"
             ["item 3"
              ["item 4"
               ["item5"
                ["item6"
                 ["item7"
                  ["item8"
                   ["item9"
                    ["item10"
                     ["item11"
                      ["item12"
                       ["item13"
                        ["item14"
                         ["item15"]]]]]]]]]]]]]]])

Illustrates the behavior more clearly:
image

I don't expect it would be terribly hard to start coloring at the beginning of an element rather than the beginning of the line. I am happy to take a crack at a PR if that would be helpful.

I noticed this while exploring coloring schemes here lambdaisland/deep-diff2#14

@greglook
Copy link
Owner

greglook commented Oct 3, 2019

Hmm, you are quite right. I don't think I'll have time to look at this in the short term, but I'd be happy to accept a PR to fix the behavior! Glad you've been getting value out of the library. 😄

@lread
Copy link
Contributor Author

lread commented Oct 3, 2019

Thanks for the reply @greglook! It turns out the issue lays in fipp: brandonbloom/fipp#66. Under the kind guidance of @brandonbloom, I've submitted a PR: brandonbloom/fipp#69.

Here's a preview of of puget with the updated fipp:
image

I shall follow up here when the fipp PR has been merged and released.

@brandonbloom
Copy link
Contributor

Fipp v0.6.21 now contains this fix.

@lread
Copy link
Contributor Author

lread commented Oct 5, 2019

This is great, thanks @brandonbloom! I'll start on PR. I am noticing that there is little in way of ansi tests so I'll beef that up as part of verification along with fipp version bump.

lread added a commit to lread/puget that referenced this issue Oct 6, 2019
Bumped fipp to 0.6.21 to pick up brandonbloom/fipp#66
which resolves greglook#44.

To verify, beefed up ansi unit tests to include a test that almost matches
example in puget README and a test that clearly shows indentation.

Of note: While developing these tests I used kaocha because it has excellent
expected vs actual diff reporting. Because kaocha uses puget, I figured it might
not be the best idea to use kaocha for puget testing and therefore have not
proposed to include it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants