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

Elixir support #1844

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft

Elixir support #1844

wants to merge 24 commits into from

Conversation

r-tae
Copy link
Contributor

@r-tae r-tae commented Sep 3, 2023

What

Adds support for the elixir programming language

Checklist

  • Recorded tests for the new language
  • Used "change" / "clear" instead of "take" for selection tests to make recorded tests easier to read
  • Added a few specific tests that use "chuck" instead of "change" to test removal behaviour when it's interesting, especially:
    • "chuck arg" with single argument in list
    • "chuck arg" with multiple arguments in list
    • "chuck item" with single argument in list
    • "chuck item" with multiple arguments in list
  • Added @textFragment captures. Usually you want to put these on comment and string nodes. This enables "take round" to work within comments and strings.
  • Added a test for "change round" inside a string, eg "hello (there)"
  • [/] Supported "type" both for type annotations (eg foo: string) and declarations (eg interface Foo {}) (and added tests for this behaviour 😊)
  • Supported "item" both for map pairs and list entries (with tests of course)
  • Add support for "name"

@r-tae r-tae changed the title Initial WIP elixir support Elixir support Sep 3, 2023
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

wow off to a fantastic start 😍! I left a bunch of small comments. Feel free to drop into a meet-up as well; we have them 3 times a week on various timezones

queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
@r-tae

This comment was marked as resolved.

data/playground/elixir/calls.ex Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
@r-tae r-tae marked this pull request as ready for review September 14, 2023 08:57
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Looking good! Left some minor comments. Also

  • There are a few empty checkboxes on PR description
  • It's worth thinking bout whether there are other scopes you want to implement. Eg "state", "map", "list", etc
  • What about items in lists? Seems like you've only done maps, but maybe I missed it
  • I see you checked box for textFragment, but I don't see that in the code
  • Looks like you're still missing a few tests, eg "change item" (I only see "chuck")

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
packages/common/src/extensionDependencies.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

we might want to support "chuck key" / "chuck value" removing the =>; might make it easier to convert a map to a list. We support that in other languagse

queries/elixir.scm Outdated Show resolved Hide resolved
queries/elixir.scm Outdated Show resolved Hide resolved
@jaresty
Copy link
Contributor

jaresty commented Jan 3, 2024

This is great! I'm about to be doing some elixir work. What's left to this? Can I help?

@r-tae
Copy link
Contributor Author

r-tae commented Jan 3, 2024

@jaresty help would be great! I haven't had a chance to do any more on this since Sept. The biggest outstanding thing is drink/pour in lists/maps (see Pokey's last comment, I haven't made it to a meetup since).

I don't remember what else is outstanding, but I think its mergable if we get just that working

@jaresty
Copy link
Contributor

jaresty commented Jan 3, 2024

@r-tae I'd love to help! Could we maybe find some time where you could introduce me to the work? I sent you a message on slack if you'd like to coordinate.

@pokey
Copy link
Member

pokey commented Jan 26, 2024

"call"

image

⬆️ I'm a bit surprised that a module definition is considered a call

image image

⬆️ It's also surprising that function definitions are considered calls

But maybe the above all technically are function calls, because they're calling def and defmodule, respectively? I guess it depends whether users would view those as function calls

image

⬆️ I'm no Elixir expert, but one_arg(x) looks like the name / arg list, not a call

@pokey
Copy link
Member

pokey commented Jan 26, 2024

"funk"

image image

Are these not function definitions?

@pokey
Copy link
Member

pokey commented Jan 26, 2024

"key"

image

doesn't look right to me; should just be a and b

image

shouldn't name and email and "name" be keys? I'm also not convinced user should be a key at all 🤔

image image

We generally prefer if removal range of key extends all the way to start of value, to make it easier to convert a map to a list

@pokey
Copy link
Member

pokey commented Jan 26, 2024

"value"

image

Looks like nested values are missing here (ie "Jane" and "jane@example.com")

image

As with "key", the removal range for value should extend from end of key

@pokey
Copy link
Member

pokey commented Jan 26, 2024

Ok I kicked the ball down the field a bit; see my changes @jaresty I'm happy to get you up to speed on this PR in a meet-up if you want to carry it to the finish line; not too much left

@jaresty
Copy link
Contributor

jaresty commented Jan 26, 2024

Thank you for that @pokey - I'm unexpectedly in a job search so it may be some time before I can get back to this.

@pokey
Copy link
Member

pokey commented Jan 26, 2024

ok @r-tae if you wanna bring this home go for it. Otherwise I may take a pass again in the future

@pokey pokey self-assigned this Jun 20, 2024
@pokey pokey removed their assignment Aug 1, 2024
@pokey pokey added the 30 mins PRs that are very close and just need a couple quick tweaks to merge; maintainer may take it label Aug 1, 2024
@jaresty
Copy link
Contributor

jaresty commented Oct 18, 2024

@r-tae wanted to let you know that community now has an elixir language mode 😄

@AndreasArvidsson
Copy link
Member

@r-tae @jaresty Would be great if any of you had the time to finish this one.
I will convert it to draft in the meantime.

@AndreasArvidsson AndreasArvidsson marked this pull request as draft January 14, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
30 mins PRs that are very close and just need a couple quick tweaks to merge; maintainer may take it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants