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 hash keys (w/ specs) #263

Closed
wants to merge 1 commit into from
Closed

Color hash keys (w/ specs) #263

wants to merge 1 commit into from

Conversation

skull-squadron
Copy link
Contributor

Supersedes #58

@gerrywastaken
Copy link
Contributor

gerrywastaken commented Aug 30, 2016

@steakknife Thanks heaps for the pull request and for updating the tests.

While testing your PR I ran into a bug that had previously slipped through. Fixed here:
#267

However your change means that code will once again not work correctly as it expects a symbol to begin with a colon. If plain_single_line starts adding color strings, then symbol? no longer will return true which will break the ruby19_syntax option. There's probably a really easy way to fix all this, but I can't think of it right now.

Also the name plain_single_line suggests options[:plain] = true and options[:multiline] = false. If we want a method that just does a options[:multiline] = false then perhaps it's better to create a new method that does only that and call it single_line leaving plain_single_line unmodified.

What are your thoughts?

@skull-squadron
Copy link
Contributor Author

skull-squadron commented Sep 7, 2016

This was a quick hack to highlight an issue. Whichever way forward colorizes / formats keys of hashes by default seems like an obvious, desirable fix. If you know how to fix / refactor it (not switch to plain for keys?) and colorize keys, by all means, please supersede this PR.

EDIT: it also looks like the colorizing code is impure and this option save/restore state may run into nasty side-effects, and might want to pass a copy of options (or state) as a parameter as it's traversing composite objects during ai.

Leaving this open as a reminder would probably be good to prevent falling between the cracks. (And a later fix would close this and #58.)

@gerrywastaken
Copy link
Contributor

@steakknife That's fine with me. I've got a few things on my plate at the moment, but I also like the idea behind this PR, so I'll come back to it, but it might not be soon. Thanks for the insight btw.

@skull-squadron skull-squadron closed this by deleting the head repository Feb 2, 2023
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