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
base: master
Are you sure you want to change the base?
Conversation
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
a90b18f
to
dd209a0
Compare
@dougwilson, that's alright!
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. |
This comment has been minimized.
This comment has been minimized.
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 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! |
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 throughcompile
, not just thedev
one? If that's the case, I'll do that separately.resolves #171