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

Remove findEnclosingTags method #381

Merged
merged 7 commits into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@50Wliu
Copy link
Member

commented Feb 28, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

findEnclosingTags had an important limitation: it checked TextMate scopes, and therefore was affected by the tokenization limit. Most code paths already used findStartEndTags instead, which skipped the scope check.
The last place that used it also hadn't been updated to support the new syntax tree method of finding tags.
Therefore, this PR:

  1. Updates the last codepath to remove its dependence on findEnclosingTags in favor of findStartEndTags and also uses that only as a fallback if tree-sitter grammars aren't being used.
  2. Removes findEnclosingTags completely.

Alternate Designs

N/A

Benefits

  • bracket-matcher:go-to-matching-bracket works on HTML files when tree-sitter is enabled
  • bracket-matcher:go-to-matching-bracket does not throw an error when there are no enclosing brackets (#380)
  • bracket-matcher:go-to-matching-bracket works on very long lines when tree-sitter is disabled

Possible Drawbacks

When tree-sitter is disabled, it may be possible to find a tag lookalike that isn't actually a tag since no scope checks are performed.

Verification Process

Tested manually and confirmed that bracket-matcher:go-to-matching-bracket no longer throws when there are no enclosing brackets. The included test fails without the included changes.

Applicable Issues

Fixes #380

50Wliu added some commits Feb 28, 2019

@50Wliu 50Wliu marked this pull request as ready for review Feb 28, 2019

@Arcanemagus
Copy link

left a comment

Changes look good to me, and the potential drawback applies to the rest of the existing code so it's now simply a limitation of the package.

@50Wliu 50Wliu merged commit 0ffadcb into master Feb 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-rm-enclosing-tags branch Feb 28, 2019

@alexchandel

This comment has been minimized.

Copy link

commented May 6, 2019

This issue has generated multiple reports on atom/atom#18160 recently, and yet despite multiple intervening Atom updates, it still has not been merged into Atom.

@Aerijo

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@alexchandel The "merged" description begs to differ. Note it was only merged a few days ago, so some patience is necessary before it makes it's way to into releases. But, theoretically, you would get this fix if you were to clone and build the repo in its current state (you'd also get all the other unreleased changes which have not been tested even in beta yet, so its not really recommended though).

You didn't specify which version of Atom you use, but this change will first be in beta, and then normally stable will get it a minor release later.

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I did forget to publish a new version. I'll do that today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.