Skip to content

Conversation

@glatzor
Copy link

@glatzor glatzor commented Jan 6, 2014

No description provided.

@ginatrapani
Copy link
Member

Hi there! Thanks for the pull request. Can you add test cases which assert this new behavior works as intended? Thanks.

@h4rrydog
Copy link

@ginatrapani This is a lovely feature. Any idea if and when this PR will be merged?

@ginatrapani
Copy link
Member

We just need tests and then I can merge.

@glatzor
Copy link
Author

glatzor commented Mar 27, 2014

On Fri, Mar 21, 2014 at 01:19:56PM -0700, Gina Trapani wrote:

We just need tests and then I can merge.

Hello Gina,

Sorry for the late reply. I am currently writing the tests
using your elegant test suite! Really a nice piece of work!

But there are two small design issues:

  • It would be nice to apply the FINAL_FILTER before the
    colorization. Otherwise the FINAL_FILTER has to deal with
    escape sequences which adds some complexity
  • Should there be any restrictions on the key names of meta data?
    Should every non whitespace character be allowed, e.g.
    meta_under:score or ☃:carrot? Or only ASCII letters?

Cheers,

Sebastian

@jrhorn424
Copy link

@glatzor Did you ever complete your tests? Why not go ahead and implement the changes you suggest as part of the PR so @ginatrapani can review.

@Dannyzen
Copy link

Thanks for this! Patched it into my fork! 👍

@karbassi
Copy link
Member

@glatzor We're reviewing all the PRs since the project has moved over to community management.

Can you move these changes into a new branch, merge our current master into the new branch (to bring it up to date), and update this PR? That way we can review and merge it.

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.

6 participants