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

replace BashColor singleton with colorize function #109

Merged
merged 7 commits into from
Dec 22, 2018
Merged

replace BashColor singleton with colorize function #109

merged 7 commits into from
Dec 22, 2018

Conversation

joshuarli
Copy link
Contributor

AFAIK the BashColor singleton exists to toggle color enable/disable for easier testing, but it can indeed be replaced with a more lightweight and reusable colorize function.

Ideally, tests should be written to compare colors so the UX can be consistent - I won't be including that in this branch, and instead opted to just strip the ANSI escape sequences away for now since that was essentially what was being done before.

Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

Great stuff!

testing/util.py Show resolved Hide resolved
tests/main_test.py Outdated Show resolved Hide resolved
@joshuarli
Copy link
Contributor Author

OK, had to fumble with lots of terrible git-fu because I partially rebase-merged with upstream master in the GitHub UI, then some more changes were posted to upstream since, and my local branch also wasn't synced with my fork. XD

@KevinHock KevinHock merged commit 1415b4b into Yelp:master Dec 22, 2018
@joshuarli joshuarli deleted the remove-bashcolor-singleton branch December 22, 2018 02:30
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.

None yet

2 participants