Skip to content

bpo-40287: Fix SpooledTemporaryFile.seek() return value #19540

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 17, 2020
Merged

bpo-40287: Fix SpooledTemporaryFile.seek() return value #19540

merged 4 commits into from
Apr 17, 2020

Conversation

methane
Copy link
Member

@methane methane commented Apr 15, 2020 •
edited by bedevere-bot
Loading

corona10
corona10 reviewed Apr 15, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

  • Are you going to submit a backported patch for 3.8/3.7 as the bug?
  • if not, don't we have to add to the documentation as Changed in version 3.9:?

@methane
Copy link
Member Author

methane commented Apr 16, 2020

  • Are you going to submit a backported patch for 3.8/3.7 as the bug?

Yes. I think this is a bug and safe enough to backport.

@methane methane added needs backport to 3.7 type-bug An unexpected behavior, bug, or error labels Apr 16, 2020
aeros
aeros approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @methane. The changes LGTM; I just have a minor suggestion regarding the Misc/NEWS entry.

Misc/NEWS.d/next/Library/2020-04-15-17-21-48.bpo-40287.-mkEJH.rst Outdated
@@ -0,0 +1 @@
Fixed ``SpooledTemporaryFile.seek()`` did not return the position.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Fixed ``SpooledTemporaryFile.seek()`` did not return the position.
Fixed ``SpooledTemporaryFile.seek()`` to return the position instead of
``None``.

Grammar fix and slight elaboration on the change made.

Copy link
Member Author

Choose a reason for hiding this comment

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

slight elaboration on the change made.

While return (including implicit return) and return None has the same behavior,
"return None intentionally" is different from "return nothing (but None actually)" for human.

In this case, returned None didn't have any meaning. So I don't want to mention about None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, good point. I suppose my thoughts were that if I were to read this news entry, I might ask "what did it do previously?". But in this case, the change was probably straightforward enough that they could simply look at the source.

corona10
corona10 previously approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM

@corona10
Copy link
Member

@methane
By the way, Can we add the unit test for the future regression?
https://github.com/python/cpython/blob/master/Lib/test/test_tempfile.py

@corona10 corona10 dismissed their stale review April 16, 2020 14:36

Unittest

corona10
corona10 approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@methane lgtm :)

@methane methane merged commit 485e715 into python:master Apr 17, 2020
@methane methane deleted the bpo40287 branch April 17, 2020 06:56
@miss-islington
Copy link
Contributor

Thanks @methane for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 17, 2020
@methane @miss-islington
bpo-40287: Fix SpooledTemporaryFile.seek() return value (pythonGH-19540)
c89009b 
It has not returned the file position after the seek.
(cherry picked from commit 485e715)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
@bedevere-bot
Copy link

GH-19566 is a backport of this pull request to the 3.8 branch.

@bedevere-bot
Copy link

GH-19567 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 17, 2020
@methane @miss-islington
bpo-40287: Fix SpooledTemporaryFile.seek() return value (pythonGH-19540)
fadb3fa 
It has not returned the file position after the seek.
(cherry picked from commit 485e715)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
miss-islington added a commit that referenced this pull request Apr 17, 2020
@miss-islington @methane
bpo-40287: Fix SpooledTemporaryFile.seek() return value (GH-19540)
11ae7d0 
It has not returned the file position after the seek.
(cherry picked from commit 485e715)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
miss-islington added a commit that referenced this pull request Apr 17, 2020
@miss-islington @methane
bpo-40287: Fix SpooledTemporaryFile.seek() return value (GH-19540)
9796fe8 
It has not returned the file position after the seek.
(cherry picked from commit 485e715)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request May 29, 2020
@CuriousLearner
Merge branch 'master' into bpo-26543
e4d193d 
* master: (1985 commits)
  bpo-40179: Fix translation of #elif in Argument Clinic (pythonGH-19364)
  bpo-35967: Skip test with `uname -p` on Android (pythonGH-19577)
  bpo-40257: Improve help for the typing module (pythonGH-19546)
  Fix two typos in multiprocessing (pythonGH-19571)
  bpo-40286: Use random.randbytes() in tests (pythonGH-19575)
  bpo-40286: Makes simpler the relation between randbytes() and getrandbits() (pythonGH-19574)
  bpo-39894: Route calls from pathlib.Path.samefile() to os.stat() via the path accessor (pythonGH-18836)
  bpo-39897: Remove needless `Path(self.parent)` call, which makes `is_mount()` misbehave in `Path` subclasses. (pythonGH-18839)
  bpo-40282: Allow random.getrandbits(0) (pythonGH-19539)
  bpo-40302: UTF-32 encoder SWAB4() macro use a|b rather than a+b (pythonGH-19572)
  bpo-40302: Replace PY_INT64_T with int64_t (pythonGH-19573)
  bpo-40286: Add randbytes() method to random.Random (pythonGH-19527)
  bpo-39901: Move `pathlib.Path.owner()` and `group()` implementations into the path accessor. (pythonGH-18844)
  bpo-40300: Allow empty logging.Formatter.default_msec_format. (pythonGH-19551)
  bpo-40302: Add pycore_byteswap.h header file (pythonGH-19552)
  bpo-40287: Fix SpooledTemporaryFile.seek() return value (pythonGH-19540)
  Minor modernization and readability improvement to the tokenizer example (pythonGH-19558)
  bpo-40294: Fix _asyncio when module is loaded/unloaded multiple times (pythonGH-19542)
  Fix parameter names in assertIn() docs (pythonGH-18829)
  bpo-39793: use the same domain on make_msgid tests (python#18698)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros aeros approved these changes

@corona10 corona10 corona10 approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@methane @corona10 @miss-islington @bedevere-bot @aeros @the-knights-who-say-ni