-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add local parser building functionality #220
base: master
Are you sure you want to change the base?
Conversation
DST-PATH specifies whether the build result should be placed. If | ||
DST-PATH is unset, it will default to the value of | ||
`tree-sitter-langs-grammar-dir' if bound. Otherwise, an error will be raised." | ||
(if-let ((dst-path (or dst-path (bound-and-true-p tree-sitter-langs-grammar-dir)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit hacky to me, but I think users will want the build directory to default to the TSL grammar dir 99% of the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about improving the situation with something like this:
(defvar tsc-dst-path-decider-func #'tsc-default-dst-path-decider
"Function to decide the destination path for building the parser.
Takes `src-path` and `dst-path` as arguments, should return the final `dst-path` to use.")
(defun tsc-default-dst-path-decider (src-path dst-path)
"Default function for deciding destination path."
(or dst-path (bound-and-true-p tree-sitter-langs-grammar-dir)))
(cl-defun tsc-build-parser-from-source (src-path &key generate dst-path)
;;;;
(if-let ((dst-path (funcall tsc-dst-path-decider-func src-path dst-path)))
@ubolonton there appears to be a regression with windows CI unrelated to this PR, tested with an empty commit here: #221 |
Windows CI used to pin Emacs version, but no longer does, after this supposedly temporary change. This likely means the test |
This PR will make it possible for users to build TS grammars locally with no external CLI dependencies, which is a big win for making that an accessible option for users.
A couple points of discussion:
tree-sitter-cli
brings in a non-trivial amount of unrelated cli-based dependencies in order to use it'stree-sitter generate
functionality. Do we think it's worth it?I'm open to any and all changes in order to make this PR easier to merge/ fit better with the codebase!
If/when this gets merged, we'd likely also want to rework tree-sitter-langs to use this function.