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

Update bindings & workflows #126

Closed
wants to merge 3 commits into from
Closed

Conversation

ObserverOfTime
Copy link
Contributor

@ObserverOfTime ObserverOfTime commented Jun 14, 2024

  • Adapt existing bindings to match the upstream ones
  • Add the new standard bindings introduced upstream
  • Add (as of yet) non-standard bindings for ktreesitter
  • Use the official upstream workflows

@github-actions github-actions bot added ci-cd CI/CD-related grammar Related to the grammar bindings Related to the Node.js/Rust/C bindings labels Jun 14, 2024
@ObserverOfTime
Copy link
Contributor Author

@fwcd @VladimirMakaev

Copy link
Owner

@fwcd fwcd left a comment

Choose a reason for hiding this comment

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

Thanks, left a few notes below

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
@fwcd
Copy link
Owner

fwcd commented Jul 29, 2024

Perhaps it would make sense to split out the update to tree-sitter 0.22.6 into a separate PR (or we could just merge #121), so we can then address the CI changes separately? I'd like some more time to review those, the deployment infrastructure can be a bit finnicky and I currently don't have the time to check that in detail.


>`npm run test`
>`npm x -- tree-sitter test`
Copy link
Owner

Choose a reason for hiding this comment

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

If we keep the scripts we can even shorten this:

Suggested change
>`npm x -- tree-sitter test`
>`npm test`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then we will lose the node package tests.

Copy link
Owner

Choose a reason for hiding this comment

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

Then we should adapt the test script to run tree-sitter test (too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather add a different script for it.

.github/workflows/regenerate-parser.yml Outdated Show resolved Hide resolved
Comment on lines 1 to 22
# Kotlin Grammar for Tree-sitter

This crate provides a Kotlin grammar for the [tree-sitter](https://tree-sitter.github.io/tree-sitter/) parsing library. To use this crate, add it to the `[dependencies]` section of your `Cargo.toml` file:

```toml
tree-sitter = "0.22"
tree-sitter-kotlin = "0.3.7"
```

Typically, you will use the `language` function to add this grammar to a tree-sitter [`Parser`](https://docs.rs/tree-sitter/*/tree_sitter/struct.Parser.html), and then use the parser to parse some code:

```rust
let code = r#"
data class Point(
val x: Int,
val y: Int
)
"#;
let mut parser = Parser::new();
parser.set_language(&tree_sitter_kotlin::language()).expect("Error loading Kotlin grammar");
let parsed = parser.parse(code, None);
```
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's redundant. The same example is in the docs.

Copy link
Owner

@fwcd fwcd Jul 29, 2024

Choose a reason for hiding this comment

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

True, but I still think having a README.md on the crate is friendlier than not having one. I would like to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has the main readme like the other packages.

@ObserverOfTime ObserverOfTime closed this by deleting the head repository Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings Related to the Node.js/Rust/C bindings ci-cd CI/CD-related grammar Related to the grammar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants