elixir-editors / elixir-tmbundle

A TextMate and Sublime Text Bundle for the Elixir programming language
https://elixir-lang.org/
Other
263 stars 61 forks source link

Various Bundle Updates #155

Closed infininight closed 6 years ago

infininight commented 6 years ago

Got a request to review this bundle for inclusion in the bundle installer for TextMate. Had a few changes that I came across during the review that I've fixed up.

Notes:

Ask if you have any concerns/questions.

josevalim commented 6 years ago

Hi @infininight, thanks a lot for this!

I have created a new tag for tm1 and this is ready to merge. I gave it a try though and I found a bug:

screen shot 2018-08-15 at 09 34 55

If there is a comment, the next line after the comment does not syntax highlight properly. I could reproduce it with any keyword (such as import, alias, etc) after a comment line. Could you please take a look?

I will give this a try the rest of the day and let you know if anything comes up. Then I think we are ready to merge it!

infininight commented 6 years ago

Which editor is this failing in?

josevalim commented 6 years ago

@infininight sorry, Sublime Text 3.

josevalim commented 6 years ago

@infininight if you have any pointers where this can be, i can also take a look. :)

infininight commented 6 years ago

OK, try that change (untested).

I am mindful of compatibility with Github and Atom but not as familiar with Sublime since they are diverging from the grammar format. \G is used as an assertion in TextMate to match the end of the last begin, this means that rule would be able to end anywhere except where it started. In this case I was able to work around it be looking ahead for the absence of the # character. Believe that should work fine in Sublime.

josevalim commented 6 years ago

@infininight beautiful. This one is fixed!

This looks good to merge for me. Shall we go ahead? :)

infininight commented 6 years ago

And a commit for fixing #100 since you said I did. ;)

(Will only work in TextMate 2 and Github.)

infininight commented 6 years ago

Looks good to me.

josevalim commented 6 years ago

Oh, I thought you had done it before. Thanks a lot!

josevalim commented 6 years ago

:heart: :green_heart: :blue_heart: :yellow_heart: :purple_heart:

infininight commented 6 years ago

And I've deployed it in the installer, give it about 12 hours to propagate and everyone should be able to see it.

Note about updates: I push updates manually. I watch all the repositories in the installer so I generally notice activity and deploy updates but if you want to push one out specifically you can @ me here or on Twitter/IRC/email, a deploy just takes a second so no bother.

josevalim commented 6 years ago

Glad to hear, thank you for the improvements and help!

binaryseed commented 6 years ago

Hi, I run Sublime Text 3 and this PR unfortunately really breaks syntax highlighting. There's a lot of changes here so I don't know how to figure out what went wrong :(

infininight commented 6 years ago

@binaryseed I just looked over all the the commits and the \G usage which was fixed is the only thing that jumped out to me. Can you post a code sample of where it breaks? Narrowing it down to a type of code will help with where to look.

There are a lot of changes here but only a handful of the commits really deal with the highlighting so should be simple enough to track down.

Edit:

To narrow it down I think it could only be one of three commits (presuming we are talking about Elixir and not EEx):

binaryseed commented 6 years ago

I walked through the commits to see what caused it...

screen shot 2018-08-26 at 10 17 45 am screen shot 2018-08-26 at 10 23 42 am screen shot 2018-08-26 at 10 25 23 am

These are just the things that are easy to notice, not sure if there's something elsewhere in other syntax...

infininight commented 6 years ago

Code in question for reference:

defmodule Absinthe.Pipeline.ErrorResult do
  @moduledoc """
  A basic struct that wraps phase errors for
  reporting to the user.
  """

  alias Absinthe.Phase

  defstruct errors: []

  @type t :: %__MODULE__{
          errors: [Phase.Error.t()]
        }

  @doc "Generate a new ErrorResult for one or more phase errors"
  @spec new(Phase.Error.t() | [Phase.Error.t()]) :: t
  def new(errors) do
    %__MODULE__{errors: List.wrap(errors)}
  end
end

More followup shortly.

infininight commented 6 years ago

So looking at this nothing is being matched incorrectly but rather the scopes have been changed to something your theme is not expecting. In TextMate you can see the scope of the caret by pressing ⌃⇧P, I presume something similar exists in Sublime so you can inspect the scopes.

The conventions TextMate uses are listed here and were at least the starting point for any of the many editors that use TextMate style scopes.

Previously the bundle used a scope of entity.name.class for class names, the only valid subsections of name are function|type|tag|section. We scope class names under entity.name.type.class in TextMate grammars it may be that the conventions are different in Sublime.

As for the TextMate bundles I track: Inspecting 258 bundles I show entity.name.type.class in use in 25 different grammars and no uses without type.

The second commit you mentioned uses another scope from the list entity.other.inherited-class for inherited classes.

Regardless of the scope conventions in Sublime I would say the theme needs to be updated since there is so much grammar sharing back and forth between the Sublime/TextMate/Atom/etc. communities.

binaryseed commented 6 years ago

Alright, I edited my theme to match the scopes and it's working properly again! Thanks for taking the time to look into this @infininight!