Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
PR Check ResultsBenchmarkLinuxWindows |
...tests/snapshots/black_compatibility@miscellaneous__docstring_no_string_normalization.py.snap
Show resolved
Hide resolved
|
It would be helpful for me if you could summarise the key points what "format docstring" includes. Having a basic understanding of the rules and not having to infer them from the code, would help me review the changes. |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Very impressive! I love it how you managed to implement this without allocating in the common case where docstrings are already formatted.
The main feedback theme is around using is_python_whitespace, trim_whitespace_start, trim_whitespace, and trim_whitespace_end instead of the rust provided trim methods to only trim valid Python whitespace.
The other part is that I don't know how to integrate this into my suppression PR xD But you can let me figure this out
...tests/snapshots/black_compatibility@miscellaneous__docstring_no_string_normalization.py.snap
Show resolved
Hide resolved
| .fmt(f) | ||
| } | ||
| } | ||
| StringLayout::DocString => format_docstring(self.string.range(), f), |
There was a problem hiding this comment.
Nit: The format_docstring function is the odd one in this file. Most other code uses Format implementations.
There was a problem hiding this comment.
i thought about this too but i didn't find a design that makes this better; i'd rather turn FormatStringContinuation into a function call.
In general, what would you think about going more into the direction of trying to make function calls (or some indirection of them) format-able?
There was a problem hiding this comment.
You can return a format_with or format_once.
|
Can you extend your test plan with the similarity index? The perfect implementation wouldn't change the similarity index, any bugs would show up in a regression. |
konstin
force-pushed
the
docstring_formatting
branch
from
f8a11fc to
972139e
Compare
konstin
force-pushed
the
impl_deref_mut_for_WithNodeLevel
branch
from
9da5459 to
23af0a6
Compare
good point: main: this branch: this is a regression, i'll investigate |
|
It could be related to the fact that we format single-quote docstrings, while Black does not. (You're welcome to change that.) |
|
black does format single quote docstrings for me. The problem was a chaperone space where there shouldn't be one, it's fixed now and the similarity scores match again; i updated the PR description. |
|
Can you document whatever you see around single-quote docstrings in the Black Deviations document, where we claim Black doesn't format them? |
|
oh that's a slightly different area, it's about formatting the surroundings of the docstring that happens in class OracleSpatialRefSys(models.Model, SpatialRefSysMixin):
"Maps to the Oracle MDSYS.CS_SRS table."
cs_name = models.CharField(max_length=68)unlike black, we format this to class OracleSpatialRefSys(models.Model, SpatialRefSysMixin):
"Maps to the Oracle MDSYS.CS_SRS table."
cs_name = models.CharField(max_length=68)It's code that i only moved around in this PR. I've added a comment but i think it's reasonable for black compatibility to fix that like a compatibility bug later. (From a style perspective, i'd turn single quoted docstrings into triple quoted docstrings) |
konstin
force-pushed
the
impl_deref_mut_for_WithNodeLevel
branch
from
23af0a6 to
39611cc
Compare
konstin
force-pushed
the
docstring_formatting
branch
from
ce6a1bf to
8b7ed41
Compare
Replace tabs with spaces Refactor Don't format escaped newline docstrings Save
konstin
force-pushed
the
docstring_formatting
branch
from
8b7ed41 to
a7dd46a
Compare
3 participants
Summary Implement docstring formatting
Test Plan Matches black's
docstring.pyfixture exactly, added some new cases for what is hard to debug with black and with what black doesn't cover.similarity index:
main:
zulip: 0.99702
django: 0.99784
warehouse: 0.99585
build: 0.75623
transformers: 0.99469
cpython: 0.75989
typeshed: 0.74853
this branch:
zulip: 0.99702
django: 0.99784
warehouse: 0.99585
build: 0.75623
transformers: 0.99464
cpython: 0.75517
typeshed: 0.74853
The regression in transformers is actually an improvement in a file they don't format with black (they run
black examples tests src utils setup.py conftest.py, the difference is in hubconf.py). cpython doesn't use black.Closes #6196