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

feat: customizable occurrence highlighting #371

Conversation

david-christiansen
Copy link
Contributor

Makes it possible to disable substring-based occurrence highlighting and use only Lean's compiler responses.

I personally found the string-based highlighting to be distracting and confusing. This change defaults to semantic-only highlighting at @mhuisi 's request, so it will be user-visible.

Makes it possible to disable substring-based occurrence highlighting
and use only Lean's compiler responses.
@david-christiansen david-christiansen force-pushed the toggleable-text-highlight branch from f171094 to f2b7e77 Compare December 6, 2023 11:05
@@ -64,6 +64,21 @@
"default": true,
"markdownDescription": "Enable eager replacement of abbreviations that uniquely identify a symbol."
},
"lean4.editor.occurrenceHighlightingMode": {
"type": "string",
"description": "Highlight occurrences using Lean's semantic information, by string matching, or both",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "Highlight occurrences using Lean's semantic information, by string matching, or both",
"description": "Highlight occurrences of a symbol using Lean's semantic information, by string matching, or both",

Maybe slightly clearer as to what it is that's occurring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally avoided this phrasing because the string-based highlighting doesn't match symbols (which is why I want to turn it off - I find it undermines the usefulness of the highlights if I can't trust them). But perhaps I'm being too pedantic here.

How do you feel about the phrasing in #377?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha! The other PR is worded well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I tried hard to thread that needle, glad it succeeded

@david-christiansen
Copy link
Contributor Author

I think #377 is strictly better than this, but I'll leave it up to a maintainer to decide what to merge and what to close.

@mhuisi
Copy link
Collaborator

mhuisi commented Jan 9, 2024

Subsumed by the now merged #377.

@mhuisi mhuisi closed this Jan 9, 2024
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.

3 participants