Fix source reindenting by using textwrap.dedent directly.
#4069
Conversation
src/_pytest/_code/source.py
Outdated
| return Source(strsrc, **kwargs) | ||
|
|
||
|
|
||
| def deindent(lines, offset=None): |
There was a problem hiding this comment.
I think we can remove offset: I've added assert offset is None to the beginning of Source.deindent and the entire suite executed without triggering the assert. Can you check please if that's the case?
There was a problem hiding this comment.
this is indeed the case, are we ok breaking this api?
There was a problem hiding this comment.
dedent is in _pytest._code.source, so I don't believe it will break any code. @RonnyPfannschmidt's concern was with the Source object, which is exposed by testdir, but here it is not the case.
@RonnyPfannschmidt please correct me if I'm wrong. 👍
There was a problem hiding this comment.
ah ok, I was also going to remove offset=None in Source.deindent as well, that's slightly closer to a break but still nobody should be calling it 😆
There was a problem hiding this comment.
i all for removing that api
Codecov Report
@@ Coverage Diff @@
## master #4069 +/- ##
==========================================
+ Coverage 94.49% 94.53% +0.04%
==========================================
Files 109 109
Lines 23837 23802 -35
Branches 2361 2348 -13
==========================================
- Hits 22524 22502 -22
+ Misses 1002 996 -6
+ Partials 311 304 -7
Continue to review full report at Codecov.
|
| def __init__(self, *parts, **kwargs): | ||
| self.lines = lines = [] | ||
| de = kwargs.get("deindent", True) | ||
| rstrip = kwargs.get("rstrip", True) |
There was a problem hiding this comment.
i believe this is a breaking api change - what where our guarantees on those internals
There was a problem hiding this comment.
This isn't an api break, the parameter is ignored (nothing is validated about **kwargs)
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
based on initial review of the necessary test changes its broken worse now
testing/code/test_source.py
Outdated
|
|
||
| lines = deindent(inspect.getsource(f).splitlines()) | ||
| assert lines == ["def f():", ' c = """while True:', " pass", '"""'] | ||
| assert lines == [" def f():", ' c = """while True:', " pass", '"""'] |
There was a problem hiding this comment.
this looks pretty much broken now
There was a problem hiding this comment.
This is the same ad the test above, but in list form, I'm pretty sure this is more correct than before (before it introduced an indentation error)
There was a problem hiding this comment.
i see what you mean, please make it a indented block with disabled black formatting and a comment that the indent of the last """ is the relative ancor
There was a problem hiding this comment.
I feel I could also delete this test since the above one covers the same behavior and is more expressive 🤷♂️
RonnyPfannschmidt
dismissed
their stale review
my missunderstanding was resolved
4 participants
Resolves #4066