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

bpo-39939: Add str.removeprefix and str.removesuffix #18939

Merged
merged 39 commits into from
Apr 22, 2020

Conversation

sweeneyde
Copy link
Member

@sweeneyde sweeneyde commented Mar 11, 2020

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM. But I am not sure about adding these method for bytearray.

Needed also updates of the documentation.

Objects/bytesobject.c Show resolved Hide resolved
@sweeneyde
Copy link
Member Author

But I am not sure about adding these method for bytearray.

Currently the bytearray api is almost a strict superset of the bytes api:

>>> set(dir(bytes)) - set(dir(bytearray))
{'__getnewargs__'}

and I didn't want to break that.

Needed also updates of the documentation.

I will work on the docs tonight and tomorrow.

@serhiy-storchaka
Copy link
Member

Thank you. All this LGTM, and I am impressed by the quality of code from a new contributor.

I do not press the "Approve" button only because I did not follow the discussion. You need an approve from a core developer more interested in this feature.

Please add also a What's New entry. And add your name in Misc/ACKS.

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
Copy link
Member Author

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'cut" --> "remove" in NEWS and whatsnew

Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
@sweeneyde sweeneyde changed the title bpo-39939: Add str.cutprefix and str.cutsuffix bpo-39939: Add str.removeprefix and str.removesuffix Mar 25, 2020
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Show resolved Hide resolved
Doc/library/stdtypes.rst Show resolved Hide resolved
@sweeneyde
Copy link
Member Author

Does someone know how to make the docs escape the slice string[:len(string)-len(suffix)]? I'm having trouble with Sphinx thinking that :len is a special kind of role.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the change. Here is my second review on the doc, which is IMO the most important part here ;-)

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Objects/bytearrayobject.c Outdated Show resolved Hide resolved
Objects/bytearrayobject.c Outdated Show resolved Hide resolved
Objects/bytearrayobject.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

Does someone know how to make the docs escape the slice string[:len(string)-len(suffix)]? I'm having trouble with Sphinx thinking that :len is a special kind of role.

Sadly, your documentation syntax is correct. It's a bug in the "make suspicious" target of Doc/Makefile:

Makefile:115: recipe for target 'suspicious' failed

We could try to fix "make suspicious". But first I suggest you to write string[:-len(suffix)] instead: it's the syntax used in the PEP 616 and it's correct. I'm not sure why you chose to explicit the string length. I expect that "make suspicious" will not complain on :- which doesn't look like a Sphinx markup.

@vstinner vstinner merged commit a81849b into python:master Apr 22, 2020
@vstinner
Copy link
Member

Congrats @sweeneyde, I merged your PR! Maybe the documentation could still be enhanced even more, but I think that it's now good enough for an alpha release ;-) I built the documentation locally to check if it is rendered correctly and it was the case. I didn't see any obvious Sphinx syntax issue.

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Fedora Stable LTO + PGO 3.x has failed when building commit a81849b.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/64/builds/656) and take a look at the build logs.
  4. Check if the failure is related to this commit (a81849b) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/64/builds/656

Failed tests:

  • test_concurrent_futures

Failed subtests:

  • test_killed_child - test.test_concurrent_futures.ProcessPoolSpawnProcessPoolExecutorTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

406 tests OK.

10 slowest tests:

  • test_concurrent_futures: 2 min 39 sec
  • test_multiprocessing_spawn: 1 min 43 sec
  • test_mailbox: 1 min 22 sec
  • test_nntplib: 1 min 21 sec
  • test_multiprocessing_forkserver: 1 min 15 sec
  • test_shelve: 1 min 3 sec
  • test_multiprocessing_fork: 1 min 3 sec
  • test_largefile: 51.6 sec
  • test_asyncio: 48.8 sec
  • test_signal: 47.1 sec

1 test failed:
test_concurrent_futures

14 tests skipped:
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_ossaudiodev test_startfile test_tix test_tk test_ttk_guionly
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_concurrent_futures

Total duration: 5 min 46 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.lto-pgo/build/Lib/test/test_concurrent_futures.py", line 130, in tearDown
    self.executor.shutdown(wait=True)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.lto-pgo/build/Lib/concurrent/futures/process.py", line 724, in shutdown
    self._executor_manager_thread_wakeup.wakeup()
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.lto-pgo/build/Lib/concurrent/futures/process.py", line 80, in wakeup
    self._writer.send_bytes(b"")
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.lto-pgo/build/Lib/multiprocessing/connection.py", line 205, in send_bytes
    self._send_bytes(m[offset:offset + size])
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.lto-pgo/build/Lib/multiprocessing/connection.py", line 416, in _send_bytes
    self._send(header + buf)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.lto-pgo/build/Lib/multiprocessing/connection.py", line 373, in _send
    n = write(self._handle, buf)
OSError: [Errno 9] Bad file descriptor

to easily remove an unneeded prefix or a suffix from a string. Corresponding
``bytes``, ``bytearray``, and ``collections.UserString`` methods have also been
added. See :pep:`616` for a full description. (Contributed by Dennis Sweeney in
:issue:`18939`.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not 18939, which is about "Venv docs regarding original python install". Shouldn't this be 39939?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's a typo error: @sweeneyde: can you please propose a PR to fix the typo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or @elazarg: Do you want to propose a PR to fix the typo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I thought it might be overkill :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops -- It looks like that's the GitHub PR number rather than the bpo number. I can't make a PR tonight so feel free to change it. If not, I can fix it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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.

7 participants