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

Duplication between helix-core/src/match_brackets.rs and helix-core/src/surround.rs #3432

Closed
A-Walrus opened this issue Aug 14, 2022 · 2 comments · Fixed by #8294
Closed
Labels
A-core Area: Helix core improvements C-enhancement Category: Improvements

Comments

@A-Walrus
Copy link
Contributor

Looking at #3357 I noticed that both match_brackets.rs and surround.rs have code that is solving what appears to be essentially the same problem, finding matching brackets. The key difference between them is that match_brackets.rs is based on tree-sitter while surround.rs is not. This means that:

  • Goto matching bracket doesn't work on regular text files
  • Inconsistent behavior between Match (m) sub commands
  • It is unclear why bracket matching is based on tree sitter while everything else isn't
  • Also duplication of logic is obviously not great.
Notice also that they both have very similar `PAIRS` arrays
// match_brackets.rs
const PAIRS: &[(char, char)] = &[
    ('(', ')'),
    ('{', '}'),
    ('[', ']'),
    ('<', '>'),
    ('\'', '\''),
    ('\"', '\"'),
];
// surround.rs
pub const PAIRS: &[(char, char)] = &[
    ('(', ')'),
    ('[', ']'),
    ('{', '}'),
    ('<', '>'),
    ('«', '»'),
    ('「', '」'),
    ('(', ')'),
];

I propose unifying the logic into one file, and making all bracket matching / surrounding commands work the same.
Thoughts?

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-core Area: Helix core improvements labels Aug 14, 2022
@dead10ck
Copy link
Member

There's also https://github.com/helix-editor/helix/blob/master/helix-core/src/auto_pairs.rs ! I haven't fully thought through all the consequences, and if it would be desirable, but I had also thought before that since the auto pairs are now fully user configurable, the matching and surround logic could use the same pairs.

@jw013
Copy link
Contributor

jw013 commented Dec 7, 2023

#8294 seems very related so mentioning it for the link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Helix core improvements C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants