init(coloring): add same coloring to all #38

georgettica · 2022-10-10T13:03:04Z

from this base I can trickle the coloring into anywhere I need

ghost · 2022-10-10T13:03:42Z

👇 Click on the image for a new way to code review

Make big changes easier — review code in small groups of related files
Know where to start — see the whole change at a glance
Take a code tour — explore the change with an interactive tour
Make comments and review — all fully sync’ed with github

Try it now!

Legend

codecov · 2022-10-10T13:04:46Z

Codecov Report

Base: 100.00% // Head: 79.68% // Decreases project coverage by -20.31% ⚠️

Coverage data is based on head (1d57a1d) compared to base (d616de5).
Patch coverage: 68.29% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##            master      #38       +/-   ##
============================================
- Coverage   100.00%   79.68%   -20.32%     
============================================
  Files            1        2        +1     
  Lines           36       64       +28     
============================================
+ Hits            36       51       +15     
- Misses           0       11       +11     
- Partials         0        2        +2

Impacted Files	Coverage Δ
httpref.go	`100.00% <ø> (ø)`
view.go	`68.29% <68.29%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

georgettica · 2022-10-10T13:05:31Z

this is a bit tangled up, and currently just shows a big blob of color, this is why I wanted a designer to look at it now :)

georgettica · 2022-10-10T13:05:41Z

related to #34

georgettica · 2022-10-11T08:23:21Z

design rought outline:
title in color
description in grey
code in special color (red / yellow / etc)

cmd/root.go

dnnrly · 2022-10-15T20:22:41Z

cmd/root.go

+	if !isStylePopulated {
+		isStylePopulated = true
+		baseRootStyle = lipgloss.NewStyle().Width(width)
 	}


I don't think we need to protect this bit with a bool. isStylePopulated isn't actually set elsewhere.

but the func is happening multiple times, I can set it each time if it's clearer

dnnrly · 2022-10-15T20:24:14Z

view.go

+)
+
+var (
+	resultStyle, nameStyle, summaryStyle lipgloss.Style


resultStyle doesn't appear to be consumed anywhere, might be worth removing it.

resultStyle was the base style before using the style I got from the caller, so removed

dnnrly · 2022-10-15T20:25:34Z

view.go

+
+func PrintResultsWithStyle(results References, rootStyle lipgloss.Style) {
+	// doing the population in a point where the width should not change anymore
+	if !areStylesPopulated {


Not sure we need this bool either.

this is a different func, thus styles need to be set here aswell

so it is required

dnnrly · 2022-10-15T20:28:19Z

view.go

+func renderStyles(baseStyle lipgloss.Style) {
+	resultStyle = baseStyle.Copy()
+	nameStyle = baseStyle.Copy().
+		Foreground(lipgloss.Color("86")).
+		Bold(true).
+		Underline(true)
+
+	summaryStyle = baseStyle.Copy()
+	r, err := glamour.NewTermRenderer(
+		// detect background color and pick either the default dark or light theme
+		glamour.WithAutoStyle(),
+		// wrap output at specific width (default is 80)
+		glamour.WithWordWrap(baseStyle.GetWidth()),
+	)
+	if err != nil {
+		// hoping this doesn't happen as most commands here suceed without issue
+		panic(err)
+	}
+	descriptionStyle = r
+}


I like how this function sets the style but not keen on the use of global scope variables. Maybe return the different styles so that can be consumed in PrintResultsWithSyle?

yeah, I can return a style but this means I need to decide each time which style is what I want.

I'll do that and we'll see how it looks

tried that and it makes me inject the styles all the way down

@dnnrly do you think it's ok to close it? if so feel free to do so

view.go

dnnrly · 2022-10-15T21:11:32Z

I don't suppose you've been able to run the unit and acceptance tests? They obviously run as part of the checks for this PR but I've not been able to get them to work locally. Have you been able to get them to work? Linting too.

$ make ci-test
go test -race -coverprofile=coverage.txt -covermode=atomic ./...
# github.com/dnnrly/httpref [github.com/dnnrly/httpref.test]
./httpref_test.go:71:19: too many arguments in call to r.Summarize
	have (number)
	want ()
./httpref_test.go:84:19: too many arguments in call to r.Summarize
	have (number)
	want ()
./httpref_test.go:92:28: too many arguments in call to r.Describe
	have (number)
	want ()
FAIL	github.com/dnnrly/httpref [build failed]
?   	github.com/dnnrly/httpref/cmd	[no test files]
?   	github.com/dnnrly/httpref/cmd/httpref	[no test files]
ok  	github.com/dnnrly/httpref/test	0.044s	coverage: [no statements] [no tests to run]
FAIL
make: *** [Makefile:76: ci-test] Error 2

georgettica · 2022-10-16T09:07:36Z

I just ran the normal tests, sry :( I'll get them to work too

georgettica · 2022-10-16T09:25:52Z

@dnnrly then why did the CI pass then?

dnnrly · 2022-10-16T09:33:29Z

I'm really not sure why. I spent some time last night trying to figure out why and didn't get anywhere.

Mind if I push a couple of changed to see if we can get to the bottom of it?

georgettica · 2022-10-16T09:41:49Z

sure!
I can always rebase and fix the conflicts later

georgettica · 2022-10-18T14:39:08Z

please do let me know when you have fixed the CI issue

dnnrly · 2022-10-19T20:40:01Z

I think I've fixed the issue but you'll have to rebase your branch on top of master.

georgettica · 2022-10-20T17:36:58Z

of course, no worries

from this base I can trickle the coloring into anywhere I need

for pretty printing the description

to make the description colorful

Co-authored-by: Pascal Dennerly <[email protected]>

- remove the if statement

georgettica · 2022-10-22T12:42:02Z

so now there is another issue, not sure exactly how to move forward from it.
line too long or something (and the message is not clear for me in the test)

PS now also the make test fails

dnnrly · 2022-10-22T21:35:42Z

I wonder if it's something to do with the formatting markup in the terminal. I'll take a look.

- Removed a test that just asserted that the formatting library works as specified.

- This is checking that the formatter is formatting again so the test clause should be removed.

dnnrly · 2022-10-22T23:04:13Z

I think I sorted the test failures (I may have cheated a little). Only thing now is to make the test coverage adequate. Maybe make PrintResultsWithStyle return an error instead?

georgettica · 2022-10-23T06:56:09Z

yeah, that is the right way of doing things and I didn't want to bubble it up.
will do it tomorrow I guess

georgettica · 2022-10-23T06:56:17Z

and thanks for the help!

dnnrly · 2022-10-23T22:08:23Z

This is more than good enough. Anything important can be dealt with in other PRs. Thanks for your help in this!!!

I'm going to be away from the computer for a little bit so I'm just merging as-is.

georgettica marked this pull request as ready for review October 14, 2022 12:13

dnnrly reviewed Oct 14, 2022

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

dnnrly requested changes Oct 15, 2022

View reviewed changes

othorotka and others added 6 commits October 22, 2022 14:54

init(coloring): add same coloring to all

1c2de2f

from this base I can trickle the coloring into anywhere I need

feat(size): make size adjustable

41266e1

chore: add glamour

59c391a

for pretty printing the description

feat: use glamour

c731aa6

to make the description colorful

change date to boolean

0cb90c4

Update view.go

52b1e7b

Co-authored-by: Pascal Dennerly <[email protected]>

georgettica force-pushed the feature/georgettica/add-coloring branch from 054c833 to 52b1e7b Compare October 22, 2022 11:54

fix(ci): align ci with the actual code

6febbde

georgettica mentioned this pull request Oct 22, 2022

glamour render panics for too long lines charmbracelet/glamour#198

Closed

feat: add lipgloss all the way up

9fa507c

- remove the if statement

dnnrly added 2 commits October 22, 2022 23:33

Update unit tests to account for formatting

92f3575

- Removed a test that just asserted that the formatting library works as specified.

Remove line length assertion

8cbfd44

- This is checking that the formatter is formatting again so the test clause should be removed.

Make sure we still test Describe

1d57a1d

dnnrly merged commit c726fe1 into dnnrly:master Oct 23, 2022

dnnrly added the hacktoberfest-accepted label Oct 23, 2022

georgettica deleted the feature/georgettica/add-coloring branch October 24, 2022 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init(coloring): add same coloring to all #38

init(coloring): add same coloring to all #38

georgettica commented Oct 10, 2022

ghost commented Oct 10, 2022 •

edited by ghost

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

georgettica commented Oct 10, 2022

georgettica commented Oct 10, 2022

georgettica commented Oct 11, 2022

dnnrly Oct 15, 2022

georgettica Oct 16, 2022

dnnrly Oct 15, 2022

georgettica Oct 18, 2022

dnnrly Oct 15, 2022

georgettica Oct 16, 2022

georgettica Oct 18, 2022

dnnrly Oct 15, 2022

georgettica Oct 16, 2022

georgettica Oct 18, 2022

georgettica Oct 23, 2022

dnnrly commented Oct 15, 2022

georgettica commented Oct 16, 2022

georgettica commented Oct 16, 2022

dnnrly commented Oct 16, 2022

georgettica commented Oct 16, 2022

georgettica commented Oct 18, 2022

dnnrly commented Oct 19, 2022

georgettica commented Oct 20, 2022

georgettica commented Oct 22, 2022

dnnrly commented Oct 22, 2022

dnnrly commented Oct 22, 2022

georgettica commented Oct 23, 2022

georgettica commented Oct 23, 2022

dnnrly commented Oct 23, 2022

init(coloring): add same coloring to all #38

init(coloring): add same coloring to all #38

Conversation

georgettica commented Oct 10, 2022

ghost commented Oct 10, 2022 • edited by ghost Loading

Legend

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

georgettica commented Oct 10, 2022

georgettica commented Oct 10, 2022

georgettica commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnnrly commented Oct 15, 2022

georgettica commented Oct 16, 2022

georgettica commented Oct 16, 2022

dnnrly commented Oct 16, 2022

georgettica commented Oct 16, 2022

georgettica commented Oct 18, 2022

dnnrly commented Oct 19, 2022

georgettica commented Oct 20, 2022

georgettica commented Oct 22, 2022

dnnrly commented Oct 22, 2022

dnnrly commented Oct 22, 2022

georgettica commented Oct 23, 2022

georgettica commented Oct 23, 2022

dnnrly commented Oct 23, 2022

ghost commented Oct 10, 2022 •

edited by ghost

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading