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

Removed Tree-Sitter from Runestone itself #296

Merged
merged 7 commits into from
Jul 15, 2023
Merged

Conversation

ActuallyTaylor
Copy link
Contributor

This pull request removes the Tree-Sitter subdirectory from Runestone in favor of a SPM dependency. Everything functions exactly the same as before.

Currently, this pull request uses the repository https://github.com/ActuallyTaylor/tree-sitter-spm which is a fork of the original Tree-Sitter repository with SPM support added. This is not ideal, as it is not the main repository of Tree-Sitter. To remedy this, I have created a pull request on Tree-Sitter itself Tree-Sitter PR #2311 that once added, will bring SPM support to the main branch.

@simonbs
Copy link
Owner

simonbs commented Jun 15, 2023

@ActuallyTaylor Hey! Thanks for opening this PR.

I'd love to get rid of the Tree-sitter submodule and depend on Tree-sitter through SPM. However, I'm a little reluctant to make that dependency on a package specification managed by a third-party, so I'll wait to see if the specification is merged into the official Tree-sitter repository before merging this PR.

@ActuallyTaylor
Copy link
Contributor Author

However, I'm a little reluctant to make that dependency on a package specification managed by a third-party, so I'll wait to see if the specification is merged into the official Tree-sitter repository before merging this PR.

That sounds great to me! I would also be reluctant to merge based on a third-party so if the specification gets merged I will update this thread.

@ActuallyTaylor
Copy link
Contributor Author

36658a6

This commit removes the dependency on my branch of TreeSitter and replaces it with the main TreeSitter repository which merged my branch Merge #2311.

Copy link
Owner

@simonbs simonbs left a comment

Choose a reason for hiding this comment

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

@ActuallyTaylor This is amazing! Thanks for putting in the work to have Tree-sitter support Swift Package Manager 🙌

I've added a comment on pinning to a specific version of Tree-sitter rather than the main branch. Is that possible?

I'm happy to merge this once the SwiftLint issues have been addressed.

Package.swift Outdated Show resolved Hide resolved
@simonbs
Copy link
Owner

simonbs commented Jul 14, 2023

I've added a comment on pinning to a specific version of Tree-sitter rather than the main branch. Is that possible?

Just realized that we can't pin against a specific version until a new version of Tree-sitter that includes the Package.swift has been published. Guess we'll have to pin against the master branch, or alternatively a specific commit, until then.

@ActuallyTaylor
Copy link
Contributor Author

Just realized that we can't pin against a specific version until a new version of Tree-sitter that includes the Package.swift has been published. Guess we'll have to pin against the master branch, or alternatively a specific commit, until then.

Hmm that is a good point, I think the best for now would be to pin to the merge commit on main until the next version of Tree-sitter is released.

@simonbs
Copy link
Owner

simonbs commented Jul 14, 2023

I think the best for now would be to pin to the merge commit on main until the next version of Tree-sitter is released.

That sounds like a good approach. Will you update this PR to do that?

@ActuallyTaylor
Copy link
Contributor Author

That sounds like a good approach. Will you update this PR to do that?

Yep I can update it now

Copy link
Owner

@simonbs simonbs left a comment

Choose a reason for hiding this comment

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

Great! Thanks for putting in the work on this! 🙏

@simonbs simonbs merged commit 60fc8f7 into simonbs:main Jul 15, 2023
2 of 3 checks passed
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.

2 participants