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

Highlightening of dates, metadata and numbers #128

Closed
wants to merge 5 commits into from

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