-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat(core): add plaintext matching fallback to tree-sitter matching #4288
Merged
Merged
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
2bb2562
bracket_matching: add plaintext matching fallback to tree-sitter matc…
alevinval 138fe38
feedback: address clippy complaints, and extract variables.
alevinval add3cd1
self-review: make the implementation more concise
alevinval 9f34f13
self-review: better variable names, improved test coverage
alevinval 9de7029
docs: make the documentation of the new function consistent with the …
alevinval dd44778
optimize: avoid wasteful decrement when closing last bracket
alevinval ac239af
self-review: use `or_else` to make the fallback more concise
alevinval 1e932a9
Apply feedback
alevinval 3370b05
Ensure cargo fmt
alevinval 9ee32af
Remove unused assertion function
alevinval f5cb382
feedback: convert some of the comments to doc-comments
alevinval 715b928
feedback: limit plain-text scanning to 10k characters at most, to avo…
alevinval adf0fbd
feedback: add plain-text fallback only when there is no syntax (tree-…
alevinval 67099d1
feedback: fix bug, use to ensure it only runs when there's no syntax
alevinval b745fb0
feedback: fix documentation
alevinval File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
One minor thing I'm noticing is that this doesn't limit the search to the current line only, as it advertises; it simply limits the scan to a max number of chars. I actually think this is fine, because limiting matching to the current line only seems unnecessarily restrictive to me, but we should probably update the function name and doc comment.
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.
yeah it was changed to a fixed limit (from only matching on the current line) after feedback from @archseer and me. I missed that the doc comment wasn't updated good catch
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.
Do you folks use something to embed a constant inside a Doc? I wanted to do this:
But that would require making the constant public for the docs to link to it correctly. If you are okay with that anyway (it works, but does not link) then I can push this
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.
That seems fine to me 👍
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.
Just remove the angle brackets SL that cargo xtask docgen doesn't complain. The link doesn't matter much. The docs are not build anyway.