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

Color Status Token #107

Closed
wants to merge 7 commits into from
Closed

Color Status Token #107

wants to merge 7 commits into from

Conversation

frankydp
Copy link

Just an additional default token for colorized status codes. Not sure if it would be wanted or useful.

Just an additional default token for colorized status codes.  Not sure if it would be wanted or useful.
@dougwilson
Copy link
Contributor

Thanks! The concept seems fine, just a few issues:

  1. The changes do not even compile. All tests are red due to the inability to now even require() the script. Please take a look into this issue.
  2. There are no tests for the added token, please add complete tests for app possible branches of the token :)
  3. There is no documentation added to the README for the token, so that would be sweet to get added as well :)

In addition to this, this PR should also DRY up the dev format as well. Probably the code for this new token should be the exact same color code from the dev format instead of some other code, and the format can just use this token and then get smashed down into just being a string like the other formats are.

@frankydp
Copy link
Author

Thanks for the feedback. I will get the details hashed out and resub.

@dougwilson
Copy link
Contributor

Cool :) Just update this pull request with the new changes and ping me when they are included and I'll take another look :)

@frankydp
Copy link
Author

@dougwilson Dried up the dev compile to the string format. Added test for new token, and corrected test for dev to account for the resulting changes to responses, left in the color test in dev even though they were a little redundant. Added colorstatus token to docs. Also, changed the color code to be more inline with the original implementation from dev. Sorry if there was any build spam.

README.md Outdated
@@ -159,6 +159,10 @@ If the request/response cycle completes before a response was sent to the
client (for example, the TCP socket closed prematurely by a client aborting
the request), then the status will be empty (displayed as `"-"` in the log).

##### :colorstatus

The colorized :status of the response.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add documentation about which statuses get which colors.

…o dev via token. Revert dev test changes. Not sure if color reset should be documented.
@artis3n
Copy link

artis3n commented Sep 11, 2016

The last comment on this was on May 28. Is this still on track to being implemented @frankydp? Seems like a revert of a removed test is still pending.

@frankydp
Copy link
Author

@arikalfus Revert was committed, with a clear added to accommodate, back on May 28th. Pull seems to have conflicts now though.

@dougwilson
Copy link
Contributor

Hi @frankydp , sorry, until very recently GitHub did not send any notice when PRs where updated, so since your last update you never posted a comment, I didn't realize you pushed anything else, very sorry for the misunderstanding!

Looking over the PR, there are still a few issues to be resolved:

  1. Every line in the :dev format seems to now be starting with a space character where before the first letter of the method was the first character. You should probably remove that space character.
  2. The documentation for :colorstatus is confusing. It sounds like it's just the colorized :status token, but if I override that token to do something different, :colorstatus does not seem to be affected.
  3. It doesn't seem like you reverted those tests that I asked earlier, as they are still showing as altered in the current diff (one line was changed and another line was removed). Can you put them back to how they were, please?

@dougwilson
Copy link
Contributor

Closing the PR since I never heard back and there are conflicts that prevent merging.

@dougwilson dougwilson closed this Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants