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

Add colored status token #174

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

Conversation

st-sloth
Copy link

Coloring status makes sense for whatever format user wants, not just the internal dev one. So using :status-colored gives that ability.

Had to edit tests because, in master, status coloring is being disabled after a space following status specifically in dev format. Now coloring is disabled right after status text.

As for collaterally removed memoization for dev format, well, wouln't it be better to memoize all formats that go through compile, not just the dev one? If that's the case, I'll do that separately.

resolves #171

@dougwilson
Copy link
Contributor

Hi @st-sloth sorry I never saw your PR. I'm currently going through the repo and cleaning up the issues / PRs which is how I found it. So I think this is a good idea, though it just needs a rebase to fix the conflicts after I merged a couple other PRs.

As for your memoize question, the compile is a public function and since memoization has no expiration and currently users could be generating a new format on every request for who knows what reason, it is unlikely to be safe to suddenly add an unbounded cache to this function.

But maybe I'm completely misunderstanding your question.

Coloring status makes sense for whatever format user wants, not just the internal `dev` one. So using `:status-colored` gives that ability.

Memoization for `dev` format is collaterally removed since now `dev` format is just a string and coloring is compiled as the `status-colored` token.

resolves expressjs#171
@st-sloth
Copy link
Author

st-sloth commented Dec 1, 2018

@dougwilson, that's alright!

As for your memoize question, the compile is a public function and since memoization has no expiration and currently users could be generating a new format on every request for who knows what reason, it is unlikely to be safe to suddenly add an unbounded cache to this function.

Yes, you are correct. I guess I didn't think about this at time of the opening of the PR.

Anyway, I rebased the branch onto the current master and resolved the conflicts. Now it's ready to be merged, if you're accepting this change.

@tahv0

This comment has been minimized.

@Inkbug
Copy link

Inkbug commented Oct 25, 2019

Any updates? I implemented this manually for my self, but it would be much more elegant if this could be in the original base.

@ryhinchey ryhinchey mentioned this pull request Feb 27, 2020
@ryhinchey
Copy link
Contributor

@st-sloth are you still working on this? I picked it up in this PR and am working on updating the tests #227

@st-sloth
Copy link
Author

@ryhinchey No, I'm done and have been waiting for a reply or merge ever since. Happy to see it picked up, revived and bettered, thank you! Good luck with your PR!

@Vivida1

This comment has been minimized.

@adamreisnz

This comment has been minimized.

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.

Add :status field with color as a default
7 participants