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.

Sign up for GitHub

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

Jump to bottom

Implement syntax highlight #70

Merged
merged 8 commits into from
Nov 14, 2017
Merged

Implement syntax highlight #70

merged 8 commits into from
Nov 14, 2017

Conversation

makenowjust
Copy link
Contributor

asciicast

@makenowjust makenowjust force-pushed the feature/syntax-highlight branch from ae1e846 to b22b61d Compare November 2, 2017 13:32
@makenowjust makenowjust force-pushed the feature/syntax-highlight branch from b22b61d to 99c696d Compare November 2, 2017 13:38
greyblake
greyblake reviewed Nov 2, 2017
View reviewed changes
Copy link
Member

@greyblake greyblake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very cool, thanks!
I've got to take a deeper look a little later

src/icr/highlighter.cr Outdated
:lib, :fun, :type, :struct, :union, :enum, :macro, :out, :require,
:case, :when, :then, :of, :abstract, :rescue, :ensure, :is_a?,
:alias, :pointerof, :sizeof, :instance_sizeof, :as, :typeof, :for, :in,
:undef, :with, :self, :super, :private, :protected, "new",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't :new be also a symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because "new" is not keyword really, it is just ident.

Copy link
Member

@veelenga veelenga Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may missed few keywords: nil, uninitialized as? etc. Found the list here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nil in SPECIAL_VALUES because it should be highlighted as value, not keyword. However uninitialized and as? is missing. Thank you!

src/icr/highlighter.cr
io << 36
when :white
io << 37
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a possibility to reuse standard library colorize ?

Copy link
Contributor Author

@makenowjust makenowjust Nov 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm colorize expert (see crystal-lang/crystal#3925) ;-) But I don't use colorize for this due to some reason.

One of the biggest reasons is colorize doesn't support outputting escape sequence without content.

@jwoertink
Copy link
Collaborator

I mean, really, what is there to say when @makenowjust submits a PR other than "Sweet!"? lol.

I guess one comment would be, can we make this optional? In a previous PR we added a config file. Maybe this is an option that's on by default, but allows the user to turn it off?

veelenga
veelenga reviewed Nov 3, 2017
View reviewed changes
src/icr/highlighter.cr
:"+", :"-", :"*", :"/",
:"=", :"==", :"<", :"<=", :">", :">=", :"!", :"!=", :"=~", :"!~",
:"&", :"|", :"^", :"~", :"**", :">>", :"<<", :"%",
:"[]", :"[]?", :"[]=", :"<=>", :"===",
Copy link
Member

@veelenga veelenga Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know there is !~ op. Nice 👍

veelenga
veelenga reviewed Nov 3, 2017
View reviewed changes
src/icr/highlighter.cr Outdated
end_highlight_class io
break
when :EOF
raise "Unterminated symbol array literal"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain when this will be raised? I can't catch this exception in ICR.

@veelenga
Copy link
Member

veelenga commented Nov 3, 2017 •
edited
Loading

I have played with it locally and started to love it. Nice job 👍

I guess one comment would be, can we make this optional? In a previous PR we added a config file. Maybe this is an option that's on by default, but allows the user to turn it off?

Maybe just introducing a cli flag --no-color is enough? The user will have to specify it each time he needs to disable colors.

@makenowjust
Copy link
Contributor Author

@veelenga I introduced a new flag --no-color to disable highlight.

@at-grandpa
Copy link

Looks very cool 👍

But, when I go back to history with the up key, the layout seems to collapse.

MakeNowJust/icr:feature/syntax-highlight

asciicast

crystal-community/icr:master

asciicast

System/Kernel/Crystal Version

$ system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: OS X 10.11.3 (15D21)
      Kernel Version: Darwin 15.3.0

...

$ crystal -v
Crystal 0.23.1 (2017-09-08) LLVM 4.0.1

@makenowjust
Copy link
Contributor Author

It is hard to fix this because the reason of this bug is macOS's libeditline bug. (And I should wrap \1 and \2 around escape sequence.)

I think that to disable highlight of prompt is better than broken history. I'll fix this. Don't be afraid, pry does not highlight a prompt also. (Maybe it is the same reason.)

Thank you @at-grandpa.

@at-grandpa
Copy link

@makenowjust
Thank you for fixing. I confirmed that it has been fixed at local machine.

@makenowjust
Copy link
Contributor Author

ping 🏓

@jwoertink
Copy link
Collaborator

This looks good to me. I'd kind of like to use that video up top as our new gif on the README, but with the message, and all the highlighting and stuff.

greyblake
greyblake approved these changes Nov 14, 2017
View reviewed changes
Copy link
Member

@greyblake greyblake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is awesome!

@greyblake
Copy link
Member

@jwoertink Would you like to take care of making the new gif?:)

@greyblake greyblake merged commit 3245d3e into crystal-community:master Nov 14, 2017
@greyblake
Copy link
Member

@veelenga It feels like this nice feature deserves it's own minor release (0.4.0), what do you think?

@makenowjust makenowjust deleted the feature/syntax-highlight branch November 14, 2017 09:27
@veelenga
Copy link
Member

@greyblake right, will prepare it soon ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veelenga veelenga veelenga approved these changes

@greyblake greyblake greyblake approved these changes

@jwoertink jwoertink jwoertink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@makenowjust @jwoertink @veelenga @at-grandpa @greyblake