Skip to content

Removed const esc sequence#39

Open
andres-lowrie wants to merge 1 commit into
junyu-w:developfrom
andres-lowrie:consolidate-color-sequences
Open

Removed const esc sequence#39
andres-lowrie wants to merge 1 commit into
junyu-w:developfrom
andres-lowrie:consolidate-color-sequences

Conversation

@andres-lowrie

Copy link
Copy Markdown

Addresses: https://github.com/DrakeW/corgi/issues/34

Offloads all terminal coloring to fatih/color


How I did testing:

# From `develop`
go build
./corgi new -t exporttest
# Make a command

./corgi export exporttest
./corgi export exporttest -t shell
mv exporttest{,_beforechanges}.json
mv exporttest{,_beforechanges}

git checkout branch
go build
./corgi export exportest
./corgi export exportest -t shell

# There should be no differences
diff exporttest exporttest_beforechanges
diff exporttest.json exporttest_beforechanges.json

@andres-lowrie

andres-lowrie commented Oct 6, 2018

Copy link
Copy Markdown
Author

Forgot this bit:

I used ripgrep to see usage of the const escape sequence:

rg "SHELL_GREEN|SHELL_RED|SHELL_YELLOW|SHELL_NO_COLOR"

Comment thread snippet/snippet.go Outdated
Comment thread snippet/step.go Outdated
@junyu-w

junyu-w commented Oct 18, 2018

Copy link
Copy Markdown
Owner

@andres-lowrie thanks for the PR, I've reviewed and overall this looks solid!

@andres-lowrie andres-lowrie force-pushed the consolidate-color-sequences branch from b24f486 to 0f9ed4b Compare December 14, 2018 18:09
@andres-lowrie

Copy link
Copy Markdown
Author

Finally got some down time (yay holidays!!)

I believe I addressed the points you mentioned. I also rebased the branch for a cleaner commit message.

Lmk if this needs anything else!

Happy Holiday!

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.

2 participants