Skip to content

Comments

Use "python" for markdown code fences in on-hover content#19082

Merged
dhruvmanila merged 1 commit intomainfrom
zb/python-hover
Jul 3, 2025
Merged

Use "python" for markdown code fences in on-hover content#19082
dhruvmanila merged 1 commit intomainfrom
zb/python-hover

Conversation

@zanieb
Copy link
Member

@zanieb zanieb commented Jul 2, 2025 •
edited
Loading

Instead of "text".

Closes astral-sh/ty#749

We may not want this because the type display implementations are not guaranteed to be valid Python, however, unless they're going to highlight invalid syntax this seems like a better interim value than "text"? I'm not the expert though. See astral-sh/ty#749 (comment) for prior commentary.

edit: Going back further to #17057 (comment) for prior context, it turns out they do highlight invalid syntax in red which is quite unfortunate and probably a blocker here.

@zanieb zanieb added ty Multi-file analysis & type inference server Related to the LSP server labels Jul 2, 2025
@zanieb
Copy link
Member Author

zanieb commented Jul 2, 2025 •
edited
Loading

I didn't test this in an editor, that seems harder than finding the place to change this literal :)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2025 •
edited
Loading

mypy_primer results

Changes were detected when running on open source projects 
beartype (https://github.com/beartype/beartype)
- TOTAL MEMORY USAGE: ~88MB
+ TOTAL MEMORY USAGE: ~97MB

black (https://github.com/psf/black)
- TOTAL MEMORY USAGE: ~142MB
+ TOTAL MEMORY USAGE: ~129MB

alerta (https://github.com/alerta/alerta)
- TOTAL MEMORY USAGE: ~117MB
+ TOTAL MEMORY USAGE: ~106MB
-     memo fields = ~97MB
+     memo fields = ~88MB

discord.py (https://github.com/Rapptz/discord.py)
-     memo fields = ~207MB
+     memo fields = ~189MB

isort (https://github.com/pycqa/isort)
- TOTAL MEMORY USAGE: ~80MB
+ TOTAL MEMORY USAGE: ~88MB

pylox (https://github.com/sco1/pylox)
-     memo fields = ~54MB
+     memo fields = ~49MB

mkosi (https://github.com/systemd/mkosi)
- TOTAL MEMORY USAGE: ~129MB
+ TOTAL MEMORY USAGE: ~117MB
-     memo fields = ~106MB
+     memo fields = ~97MB

schemathesis (https://github.com/schemathesis/schemathesis)
- TOTAL MEMORY USAGE: ~171MB
+ TOTAL MEMORY USAGE: ~156MB

vision (https://github.com/pytorch/vision)
-     memo fields = ~304MB
+     memo fields = ~276MB

tornado (https://github.com/tornadoweb/tornado)
-     memo fields = ~129MB
+     memo fields = ~142MB

freqtrade (https://github.com/freqtrade/freqtrade)
-     memo fields = ~304MB
+     memo fields = ~276MB

scikit-learn (https://github.com/scikit-learn/scikit-learn)
- TOTAL MEMORY USAGE: ~652MB
+ TOTAL MEMORY USAGE: ~717MB

@zanieb zanieb force-pushed the zb/python-hover branch from 59a7248 to e6f19f4 Compare July 2, 2025 02:39
@zanieb zanieb marked this pull request as ready for review July 2, 2025 03:01
@zanieb zanieb marked this pull request as draft July 2, 2025 03:23
@sharkdp
Copy link
Contributor

sharkdp commented Jul 2, 2025

edit: Going back further to #17057 (comment) for prior context, it turns out they do highlight invalid syntax in red which is quite unfortunate and probably a blocker here.

I tried in VS Code and everything looks kind of nice, actually. I don't see any invalid syntax highlighting.

image

image

image

@dhruvmanila
Copy link
Member

dhruvmanila commented Jul 2, 2025

I guess that would be dependent on the color scheme.

Regardless, I think this should be fine. I'm not even sure whether the red color is coming from it being a syntax error. For the color scheme that I use (Gruvbox), the keywords are highlighted as red, so I only see the red color for keywords e.g., in <class 'Foo'> the "class" is red colored but it's not due to the syntax error.

@zanieb
Copy link
Member Author

zanieb commented Jul 2, 2025

I will defer to ya'll for sure.

@zanieb zanieb marked this pull request as ready for review July 2, 2025 13:22
carljm
carljm approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

If the results here aren't obviously bad, then I think we should go ahead and do it. It's clearly what we want, and it will help us find and motivate us to fix any specific cases where our type display doesn't work well with Python syntax highlighting.

@dhruvmanila dhruvmanila merged commit 9fc04d6 into main Jul 3, 2025
36 checks passed
@dhruvmanila dhruvmanila deleted the zb/python-hover branch July 3, 2025 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@dhruvmanila dhruvmanila dhruvmanila approved these changes

@MichaReiser MichaReiser Awaiting requested review from MichaReiser MichaReiser is a code owner

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood AlexWaygood is a code owner

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

@dcreager dcreager Awaiting requested review from dcreager dcreager is a code owner

Assignees

No one assigned

Labels

server Related to the LSP server ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

In LSP returned markdown content could have python language identifier

4 participants

@zanieb @sharkdp @dhruvmanila @carljm