Skip to content

Allow to format signatures in docstrings #631

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Mar 31, 2025

This is a proposal for #630. It comes down to:

  • adding pylsp_signatures_to_markdown hook which takes a list of strings (signatures) and returns a single (markdown) string
  • reading the hook value and passing it down as signatures_to_markdown to format_docstring

Using a list → str approach allows the hook to modify how alternative signatures are displayed.

There are no user-facing changes out of the box, however users can now install pylsp-ruff-signature:

pip install pylsp-ruff-signature

to see much more readable function signatures in hover and in completion documentation, for example:

Without pylsp-ruff-signature With pylsp-ruff-signature
image image

@krassowski krassowski marked this pull request as ready for review March 31, 2025 14:20
@krassowski
Copy link
Member Author

krassowski commented Apr 4, 2025

Any thoughts @ccordoba12 ? It would be amazing to get it in to allow downstreams to customize.

@krassowski
Copy link
Member Author

A gentle ping :)

@dalthviz dalthviz self-requested a review April 23, 2025 18:24
@krassowski krassowski marked this pull request as draft May 9, 2025 19:43
@krassowski krassowski marked this pull request as ready for review May 9, 2025 20:34
mscolnick pushed a commit to marimo-team/marimo that referenced this pull request May 12, 2025
## 📝 Summary

- formats signatures using ruff so that they are easier to read
- depends on python-lsp/python-lsp-server#631

## 🔍 Description of Changes

Adds `pylsp_signatures_to_markdown` hook, re-using existing `ruff`
formatting utility.

| Before | After |
|--|--|
|
![image](https://github.com/user-attachments/assets/2ffb4e27-6a26-4f69-ae7a-ed51faa5afc2)
| ![Screenshot from 2025-05-09
22-01-22](https://github.com/user-attachments/assets/56bde23c-193d-4800-a2c6-81b80ab72025)
|

Note: this does not help with `pd.read_csv`; I am not sure why it is not
hitting this code path, but notably it happens on the completion items
which do not have a docstring either.

## 📋 Checklist

- [x] I have read the [contributor
guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md).
- [ ] For large changes, or changes that affect the public API: this
change was discussed or approved through an issue, on
[Discord](https://marimo.io/discord?ref=pr), or the community
[discussions](https://github.com/marimo-team/marimo/discussions) (Please
provide a link if applicable).
- [ ] I have added tests for the changes made.
- [x] I have run the code and verified that it works as expected.

## 📜 Reviewers

@mscolnick
@ccordoba12
Copy link
Member

ccordoba12 commented May 12, 2025

Any thoughts @ccordoba12 ? It would be amazing to get it in to allow downstreams to customize.

Sorry for the delay in replying @krassowski. For now I only have one question for you: instead of using a hook for this (pylsp_signatures_to_markdown), why not adding the functionality to format signatures to markdown to the server itself?

The problem with your current approach is that only users that know that they need to install pylsp-ruff-signature (or a similar plugin), will be able to get better signatures. That would be ok for JupyterLab-LSP users because you could add that plugin as a new dependency. But it wouldn't be easily discoverable for users from other editors/IDEs.

I know that would add a new dependency in either Ruff or Black, but I'm ok with that. I'd prefer Black because it's pure Python, but we could look for Ruff being on PATH and use it first if found. Another option would be to create a new package (similar to docstring_to_markdown) and use it for this purpose. Then, Black/Ruff would be its dependency.

@krassowski
Copy link
Member Author

krassowski commented May 12, 2025

instead of using a hook for this (pylsp_signatures_to_markdown), why not adding the functionality to format signatures to markdown to the server itself?

I think there are two cons:

  • it adds a new dependency (say ruff or black) - you addressed this point
  • users may want to configure it (e.g. max line length, how to present typing, etc)

If you are ok with having this in pylsp and including the config options then I think would be fine.

I think we could even get away with having ruff/black as optional dependencies by introducing this plugin as disabled by default (and sniffing if ruff/black are installed).

@ccordoba12
Copy link
Member

ccordoba12 commented May 12, 2025

users may want to configure it (e.g. max line length, how to present typing, etc)

Right, I didn't thought about it.

If you are ok with having this in pylsp and including the config options then I think would be fine.

Sure, no problem about it if that'd benefit more users.

I think we could even get away with having ruff/black as optional dependencies by introducing this plugin as disabled by default (and sniffing if ruff/black are installed).

That'd be inconsistent to users because with no ruff/black they'd get different results than without them and it'd be hard for them to understand why. So, I'd say let's go with black and leave ruff as optional. In that case, the UX would be practically indistinguishable since both give pretty much the same result.

@dalthviz
Copy link
Contributor

Just in case, gave a check to the implementation and current approach (using Jupyterlab + this PR + pylsp-ruff-signature) and I think things are working as expected 👍

Regarding the implementation approach to follow, the idea then is to include python-ruff-signature as a bundled plugin for python-lsp-server (leaving it enabled by default) + allowing it to be configurable to work with either black or ruff and other configs that could be useful (max line length, typing represetation, etc) + add black as a required dependency for python-lsp-server while ruff would be optional, right?

@ccordoba12
Copy link
Member

ccordoba12 commented May 12, 2025

Regarding the implementation approach to follow, the idea then is to include python-ruff-signature as a bundled plugin for python-lsp-server

Nop, it's to implement the functionality of that plugin directly in _utils/format_docstring. Then a new hook, changes to existing hooks and a new plugin won't be necessary.

allowing it to be configurable to work with either black or ruff and other configs that could be useful (max line length, typing represetation, etc)

Yep, although I think that's a bit overkill.

add black as a required dependency for python-lsp-server while ruff would be optional, right?

Right.

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.

4 participants