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

Adding Language Support for Go #391

Merged
merged 15 commits into from
Dec 17, 2021
Merged

Adding Language Support for Go #391

merged 15 commits into from
Dec 17, 2021

Conversation

trace-andreason
Copy link
Contributor

I haven't recorded any test cases yet, but everything seems to be working. The only thing that is missing is class and class name as go does not have classes.

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.

Looks really good! Left a few comments. Let me know if you have any trouble with the test case recorder 😊

src/languages/go.ts Outdated Show resolved Hide resolved
src/languages/go.ts Outdated Show resolved Hide resolved
src/languages/go.ts Outdated Show resolved Hide resolved
src/util/nodeSelectors.ts Outdated Show resolved Hide resolved
src/util/nodeSelectors.ts Outdated Show resolved Hide resolved
src/util/nodeMatchers.ts Outdated Show resolved Hide resolved
@pokey pokey marked this pull request as draft December 14, 2021 17:12
@pokey pokey changed the title [DRAFT] Adding Language Support for Go Adding Language Support for Go Dec 14, 2021
@trace-andreason trace-andreason marked this pull request as ready for review December 15, 2021 16:55
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 👏! Fantastic first PR 😄

There are a few more tests I'd love to see:

  • "chuck arg" when there are two args in the arg list, to ensure that comma is properly cleaned up on removed arg
  • "chuck arg" when there is one arg in the arg list, just to make sure delimiter code doesn't get tripped up
  • "take every key" to ensure that sibling expansion works properly for keys
  • "take every item" to ensure that sibling expansion works properly for items
  • "take key" where the cursor is in the key itself
  • "take value" where the cursor is in the value itself
  • "take round" with a document like so: "(hello)", and cursor in the middle of hello. Will only work if you're on develop branch of cursorless-talon
  • "clear if state"
  • "clear condition"

Also, in other languages, we're able to say "take value" to select the value of a return statement. Would that be useful in Go in your opinion?

@@ -0,0 +1,23 @@
languageId: go
Copy link
Member

Choose a reason for hiding this comment

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

Another minor one for the future; not worth rewriting your tests, but we generally try to use "clear" for modifier tests so that the test case is easier to read; don't have to count character indices 😅. Not a big deal because in the future we'll have a better format for modifier tests that doesn't depend on which action you run

@pokey
Copy link
Member

pokey commented Dec 16, 2021

Also, would be great if you could file a PR against https://github.com/pokey/vscode-parse-tree for the parse tree support

@trace-andreason
Copy link
Contributor Author

@pokey
Copy link
Member

pokey commented Dec 16, 2021

@pokey isn't go already in there?

https://github.com/pokey/vscode-parse-tree/blob/main/src/extension.ts#L20

Ohhh ok. That's how you did it 😂. Forgot go was already supported by that extension

@trace-andreason
Copy link
Contributor Author

@pokey ok, added almost all those tests. I didn't add these two:

"take round" with a document like so: "(hello)", and cursor in the middle of hello. Will only work if you're on develop branch of cursorless-talon
"clear condition"

Do I have to add the first? I based my branch on main.

For the second, I can't get the condition action to register in talon, I'm not sure why.

@@ -48,6 +49,8 @@ const languageMatchers: Record<
> = {
c: cpp,
cpp,
csharp: csharp,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like issue with resolving merge conflict

Suggested change
csharp: csharp,

@pokey pokey merged commit d131a50 into cursorless-dev:main Dec 17, 2021
@pokey
Copy link
Member

pokey commented Dec 17, 2021

@pokey ok, added almost all those tests. I didn't add these two:

"take round" with a document like so: "(hello)", and cursor in the middle of hello. Will only work if you're on develop branch of cursorless-talon "clear condition"

Do I have to add the first? I based my branch on main.

For the second, I can't get the condition action to register in talon, I'm not sure why.

no worries; I just added them myself. Thanks for all the great work! This should be released in the next couple days

@trace-andreason
Copy link
Contributor Author

Thanks @pokey and @AndreasArvidsson! I'm planning to do rust next!

@AndreasArvidsson
Copy link
Member

Good work! :)

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.

3 participants