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

feat: module attributes #215

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Conversation

biletskyy
Copy link
Contributor

resolves #86

Parses AST to collect module attribute definitions. Also parses AST to get attribute reference name.

@biletskyy
Copy link
Contributor Author

tested in VSCode

module_attributes.mp4

Copy link
Collaborator

@mhanberg mhanberg left a comment

Choose a reason for hiding this comment

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

This is really, really good! Thank you so much for the contribution.

I left some comments, but they should be easy to address. I'd actually like to get this merged in quickly because you have introduced a pattern that I think will help with #104 and #152.

lib/next_ls/runtime/sidecar.ex Outdated Show resolved Hide resolved
Comment on lines 60 to 63
|> Enum.filter(&match?({:attribute, _, _, _}, &1))
|> Enum.reject(fn {_, "@" <> name, _, _} ->
Map.has_key?(Module.reserved_attributes(), String.to_atom(name))
end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For optimum efficiency, i'd combine these so its operated in a single pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, agree. Will fix that.

lib/next_ls/runtime/sidecar.ex Outdated Show resolved Hide resolved
lib/next_ls/runtime/sidecar.ex Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be helpful to extra a module for single use AST functions, to get them out of this sidecar process, as it's just meant to be a sidecar/proxy.

You can then unit test them if you'd like (I don't think it's necessary for this PR, but as we discover more complicated edge cases, would be useful)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's good idea. I extracted it. I'm not happy about module design but I think it could be refactored later with more usecases.
I'm not sure about naming so if you like call it differently I don't mind

@mhanberg mhanberg enabled auto-merge (squash) September 15, 2023 19:33
@mhanberg mhanberg merged commit b14a09d into elixir-tools:main Sep 15, 2023
3 checks passed
This was referenced Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[definition] module attributes
2 participants