Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix the color wrap function #186

Merged
merged 1 commit into from Sep 6, 2013

Conversation

Projects
None yet
3 participants
Contributor

cattode commented Sep 3, 2013

The wrap color function isn't working properly, allowing invalid color names for color and not taking reset_color into account.
This commit forces input validation against the list of color codes: if color is an invalid color entry, the function returns only the unmodified text; if reset_color is invalid, it is set to 'reset'.

Contributor

qsheets commented Sep 4, 2013

Welcome to GitHub, @cattode! For further reference, please also state what was done to "fix" the issue in your summary and it doesn't hurt to go in depth a little more. For example, in this case one might say the following:

The wrap color function isn't working properly allowing invalid color names for both color and reset_color. This commit forces input validation against the list of color codes, returning only the unmodified text upon receiving invalid color entries.

A statement like this not only states the issue clearly, it also states the intended result of the solution. This way even if this commit isn't pulled, the repository owner will still be notified of the issue and made aware of a potential solution (especially for more complex issues).

Contributor

cattode commented Sep 4, 2013

Yeah sorry, you're right, I don't use github often so I'm not very used to making pull requests yet. I've edited my message according to your recommendations!

Owner

martynsmith commented Sep 6, 2013

Yeah, it boggles how the original version even got there, this definitely looks like a step forward, thanks :-)

martynsmith added a commit that referenced this pull request Sep 6, 2013

Merge pull request #186 from cattode/wrap-patch
Fix the color wrap function

@martynsmith martynsmith merged commit 83d42a3 into martynsmith:master Sep 6, 2013

@cattode cattode deleted the cattode:wrap-patch branch Sep 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment