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

#downcase! directive to use with ruby injections #1190

Merged
merged 1 commit into from Jun 4, 2021
Merged

#downcase! directive to use with ruby injections #1190

merged 1 commit into from Jun 4, 2021

Conversation

DerekStride
Copy link
Contributor

@DerekStride DerekStride commented Apr 15, 2021

What

This is a follow-up to neovim/neovim#14152 which was superseded by neovim/neovim#14046 which allows setting the injected parser language on the metadata field of directives.

Add a downcase! directive

I moved the implementation of the #downcase! directive I proposed in neovim/neovim#14152 to this plugin as it seems to be a better home for it.

Often times the text matched by an @language node may not exactly resemble the name of the parser for that language. More often than not the parser name will be lowercase, i.e. html instead of HTML. In the below example ruby code a string constant is defined using ruby's heredoc syntax. The coding conventions for heredocs are to surround the string content with the name of the language in all uppercase letters ie. HTML, SQL, JAVASCRIPT, etc.

class A
  MY_HTML = <<~HTML
    <title>This is a title</title>
    <div class="example-class">
      <span>This is a span</span>
    </div>
  HTML
end

The downcase! directive is used in an queries/ruby/injections.scm file like so to convert the HTML value in the @language node to a valid lowercase html name of the parser.

(heredoc_body
 (heredoc_content) @content
 (heredoc_end) @language
 (#set! "language" @language)
 (#downcase! "language"))

Screenshot

Screen Shot 2021-04-15 at 15 48 00

@stsewd
Copy link
Member

stsewd commented Apr 15, 2021

Thanks for the PR, I think we could do something more general like #812

@theHamsta
Copy link
Member

@stsewd but isn't #812 capturing a node irrespective of case and this PR about processing a result of a capture? When we capture the string HTML/PYTHON we need to convert it to html/python only those are parser languages. We could also make the behavior of downcase! the default for language injection

@stsewd
Copy link
Member

stsewd commented Apr 16, 2021

Both are about capturing a string from a node and matching it to a set of languages, for example the HTML parser would have an injection regex that's case-insensitive. But I agree we can just make it default to make it lowercase.

Comment on lines 91 to 116
query.add_directive('downcase!', function(match, _, bufnr, pred, metadata)
local node = match[pred[2]]
local text = query.get_node_text(node, bufnr)
local language = string.lower(text)
metadata.language = language
end)
Copy link
Member

Choose a reason for hiding this comment

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

Although I agree with the implementation, it may be better to make it a bit more generic, maybe by providing the key to set instead of just using language this would make it look a bit like set, and may feel a little better.

Copy link
Member

Choose a reason for hiding this comment

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

@DerekStride vigoux is right. Could you add an argument that determines which metadata to downcase?

Copy link
Member

@theHamsta theHamsta Apr 16, 2021

Choose a reason for hiding this comment

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

It could even be a modifier for set! In a future PR (set! "language" @foo downcase!) or directly apply on a key (set! "language" @foo) (downcase! "language") to chain multiple functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I liked the suggestion to allow the directives to be chained, and made the corresponding changes.

(#set! "language" @foo) (#downcase! "language")

@theHamsta theHamsta requested a review from vigoux April 19, 2021 18:03
queries/ruby/injections.scm Outdated Show resolved Hide resolved
@vigoux
Copy link
Member

vigoux commented Apr 19, 2021

Hehe you'll say I am asking many things, but I'd rather go for a mimic of the signature of the set predicate.

The reason is that we don't have any guarantees on the order of evaluation of the directives, and thus making them standalone is better.
Does it sound reasonable to you?

@DerekStride
Copy link
Contributor Author

DerekStride commented Apr 19, 2021

I'd rather go for a mimic of the signature of the set predicate

I assume you mean to mimic the difference of when there are 3 vs 2 arguments, like what's listed below:

  ["set!"] = function(_, _, _, pred, metadata)
    if #pred == 4 then
      -- (#set! @capture "key" "value")
      metadata[pred[2]][pred[3]] = pred[4]
    else
      -- (#set! "key" "value")
      metadata[pred[2]] = pred[3]
    end

Then we can support (#downcase! @capture "key") too?

```
(#downcase! "language")
```

downcase! will ensure the metadata value for the specified key will be
downcased. If the value is a node, it will downcase the text specified
by the node.

```
(#downcase! @node "key")
```

You can also namespace the key with a specific capture, similar to how
you can call `(#set! @node "key" "value")`
Comment on lines +94 to +102
if #pred == 3 then
-- (#downcase! @capture "key")
key = pred[3]
value = metadata[pred[2]][key]
else
-- (#downcase! "key")
key = pred[2]
value = metadata[key]
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vigoux I made the changes that we discussed, I think I found a bug with how set was implemented in neovim. I'll be opening a PR with a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix is up here: neovim/neovim#14418

@steelsojka
Copy link
Contributor

steelsojka commented Apr 23, 2021

I think a better solution would be something more generic that could simply look up the language based on a set of rules. For example, looking it up by a table. Since we can configure the language through a directive we could make it look it up any way we want to, whether that's by matching a regex pattern, a simple table lookup, or a transformation function of the language.

EDIT: Maybe we'll just save the more advanced stuff for another time...

@steelsojka
Copy link
Contributor

@vigoux Is the order of the directives not guaranteed? I have some work that relies on that... 😢

@vigoux vigoux added this to Review in progress in Core Apr 23, 2021
stsewd added a commit to stsewd/nvim-treesitter that referenced this pull request Apr 23, 2021
@stsewd
Copy link
Member

stsewd commented Apr 23, 2021

I have implemented what I mean in #1190 (comment) #1238.

@vigoux
Copy link
Member

vigoux commented Apr 24, 2021

@vigoux Is the order of the directives not guaranteed? I have some work that relies on that... 😢

I'll dig in tree-sitter at some point, but I am not sure it is...

@theHamsta
Copy link
Member

@vigoux is this ready to merge?

@vigoux
Copy link
Member

vigoux commented Jun 4, 2021

Yup, looks ready indeed.

@vigoux vigoux merged commit e98e2ea into nvim-treesitter:master Jun 4, 2021
Core automation moved this from Review in progress to Done Jun 4, 2021
@DerekStride DerekStride deleted the downcase-directive branch June 7, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Core
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants