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

[formatter] Indent docstrings using configured indent_style #9173

Closed
wants to merge 1 commit into from

Conversation

sciyoshi
Copy link
Contributor

@sciyoshi sciyoshi commented Dec 18, 2023

Summary

Ruff currently always formats docstrings using spaces for indentation. However, when indent-style is set to "tab", this leads to E101 (mixed indentation) linter errors.

There are a few approaches to improving the situation, some of which are discussed in #8430. One option which is likely the simplest is to recommend turning off E101 when using the formatter. If this is the preferred route, then Ruff should probably also respect the indent-width when expanding tabs in docstrings to avoid the issue described here.

This PR implements an alternative solution which indents docstrings using the configured indent-style. In cases where docstrings have indents that aren't a multiple of the indent-width, this may mean E101 is still triggered in the formatted code. However, this should be very uncommon. I did a cursory check of the few codebases across GitHub that use Ruff with tabs and could not find any cases where docstrings would be negatively affected by this change.

Note that this means that Ruff will format code with tabs differently than Black, which always expands tabs to 8 spaces. I think this is acceptable in this case, because Black doesn't support tabs to begin with (so there's no real compatibility issue).

There is also the question of how other tooling treats tabs in docstrings. As of Python 3.13, docstrings will be dedented to save memory. However, tabs will also unfortunately be expanded to 8 spaces by the interpreter, which will negate the memory usage savings, but this difference should also be mostly inconsequential (and this may potentially be fixed in CPython in the future.)

Closes #8430

Copy link
Contributor

github-actions bot commented Dec 18, 2023

ruff-ecosystem results

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 2 project errors)

sphinx-doc/sphinx (error)

ruff format --no-preview --exclude tests/roots/test-pycode/cp_1251_coded.py

ruff failed
  Cause: Selection of unstable rules without the `--preview` flag is not allowed. Enable preview or remove selection of:
	- FURB113
	- FURB131
	- FURB132

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to parse examples/dalle/Image_generations_edits_and_variations_with_DALL-E.ipynb:3:7:8: Unexpected token 'prompt'

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to parse examples/dalle/Image_generations_edits_and_variations_with_DALL-E.ipynb:3:7:8: Unexpected token 'prompt'

@MichaReiser MichaReiser added formatter Related to the formatter needs-decision Awaiting a decision from a maintainer labels Dec 18, 2023
@MichaReiser
Copy link
Member

Thank you for this PR and pushing for another decison.

I don't feel comfortable making this decision right now because I don't have a good enough understanding of docstring handling in Python and I want to avoid the situation where we change the definition only to later change it again. The PR also seems to change docstring formatting when using indent-style=space, meaning this PR reduces black compatibiliity. This doesn't mean we shouldn't move forward with this PR, it's just that I need to find time time to go through the issues and familiarize myself with Python's docstring handling. I'm happy to move forward with this PR if someone else from the team has enough context and feels comfortable making the decision.

Note: We need to double check how this affects code block formatting inside docstrings (@BurntSushi)

@MichaReiser MichaReiser removed the needs-decision Awaiting a decision from a maintainer label Feb 6, 2024
@MichaReiser MichaReiser self-assigned this Feb 6, 2024
@MichaReiser
Copy link
Member

I'm now looking into this and plan to make a decision tomorrow.

@MichaReiser
Copy link
Member

MichaReiser commented Feb 8, 2024

Thanks, @sciyoshi, for putting the work into this and pushing for a decision.

I understand that changing tabs in docstrings to spaces can be surprising if a specific documentation convention uses them. Unfortunately, changing spaces to a tab isn't safe when the spaces are used for alignment.

I believe the proper fix is to add support for docstring formatting (that supports the different conventions). This way, we avoid the risk of breaking alignment and match the expectation of using tabs in these cases.

See #8430 (comment) for an in-depth explanation.

@MichaReiser MichaReiser closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter: Uses space indentation in docstrings when using indent-style = "tab"
2 participants