-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
gh-103997: Automatically dedent the argument to "-c" #103998
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
Conversation
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
71c6b63
to
51c1320
Compare
I was able to cobble my way through the C docs and write what I think is a reasonable attempt at a pure C dedent function (I forgot how fun -- albiet time consuming --- pointer logic can be). To highlight some of the corner cases that need to be accounted for, this is the test case I'm using locally:
I still haven't handled tabs, but I think my space logic is correct. I do need help vetting my C code and fixing the memory and security problems with it. |
Delete all your Delete all your |
You should act fast since 3.12 release window will soon close. No new feature after May 8. Can you give me write access to your branch? |
06667b7
to
9649590
Compare
@sunmy2019 I gave you access to my cpython fork. |
I got one thought:
It requires remembering the exact "space and tab prefix". Should we implement this? |
Probably, but off the top of my head I'm not sure what the most elegant way to do it would be. I suppose instead of maintaining the tab and space counts, it would be sufficient to maintain the current shortest whitespace sequence, and then check how much of that sequence new lines match, shortening the sequence as necessary. I can give that a try. On a different note, I'm sure that new features will want tests associated with them, but I'm not sure where the other "-c" tests live. Do you know where the best spot to put tests would be? |
@Erotemic, take a look. I have implemented it with production quality at d336ac7. Edit your #103998 (comment) to make it clear and concise, so that we can attract more reviewers. |
I guess |
We need someone with write access to review this. @vstinner as the recent maintainer of this file. |
📜🤖 Added by blurb_it.
@AA-Turner CI issues have been resolved. Since this patch was last updated the "Core and Builtins" news section had spaces in its path name removed, so the file for this patch had to be moved to the new correct location. The other issue was a c compiler warning, which was elevated to an error by CI policy. The usage of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broadly looks fine to me, a minor suggestion on the test cases. I'll leave @methane or someone else to merge, though.
A
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the refcount.dat
file (don't remember if it's in Tools, Doc or somewhere else).
Misc/NEWS.d/next/Core_and_Builtins/2023-04-29-23-15-38.gh-issue-103997.BS3uVt.rst
Show resolved
Hide resolved
Misc/NEWS.d/next/Core_and_Builtins/2023-04-29-23-15-38.gh-issue-103997.BS3uVt.rst
Show resolved
Hide resolved
Misc/NEWS.d/next/Core_and_Builtins/2023-04-29-23-15-38.gh-issue-103997.BS3uVt.rst
Show resolved
Hide resolved
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Ah, I've deleted a comment by mistake. My comment was about the construction |
I have made the requested changes; please review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-c
option in idle and pdb should be dedented too. But it can be separated PR.
Misc/NEWS.d/next/Core_and_Builtins/2023-04-29-23-15-38.gh-issue-103997.BS3uVt.rst
Show resolved
Hide resolved
I overlooked this. I will check it later. |
It's fine, this is just for internal knowledge |
This is an implementation of the idea proposed in #103997.
It intercepts the argument passed to
-c
, and removes common leading whitespace from each line in the argument.Given an input string, the algorithm overview is:
pymain-run-command
function with that.Big thanks to @sunmy2019 who really helped clean this PR up.