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

Removed const esc sequence #39

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

andres-lowrie
Copy link

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
Copy link
Author

andres-lowrie commented Oct 6, 2018

Forgot this bit:

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

rg "SHELL_GREEN|SHELL_RED|SHELL_YELLOW|SHELL_NO_COLOR"

snippet/snippet.go Outdated Show resolved Hide resolved
snippet/step.go Outdated Show resolved Hide resolved
@junyu-w
Copy link
Owner

junyu-w commented Oct 18, 2018

@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
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