GeezerVison

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:

1. Sign the PSF contributor agreement. The "bugs.python.org username" requested by the form is the "Login name" field in "Your Details" at b.p.o
2. Wait at least one US business day and then check the "Contributor form received entry under "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
3. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

@Haypo I believe I've addressed everything!
If there's anything I'm missing please let me know :)

@Haypo hopefully 3rd times the charm ;)

If you'd like me to squash the history to one commit let me know!

@terryjreedy the only problem with that is that we'll need to have the if length checks in all consuming functions as opposed to only once in copyfileobj no? Maybe you have a better approach?

Yes I totally agree about the test. I guess it goes here?

Note: I created PR #4293 to replace KB with KiB in the Python code base ;-)

I think everything has been addressed except for @terryjreedy's:

Suggestion: leave API of copyfileobj should alone as 'length=16*1024' to document default. For other functions, say something like 'If length not passed, copyfileobj uses its default.' We might change it someday.

I don't know how to keep the default value in copyfileobj but also support it as a default argument in all client functions without length = 16 * 1024 if length is None else length in each case. Personally I think that's a bit uglier. I can also stick the default value in each client function to be length=16 * 1024 and always pass it through transparently if that is preferred; it just means changing the value in multiple places if it is changed in the future. I had originally opted for one change in one place if it was to be modified later.

Whatever you all think is best I'll do 👍

Just to clear this up since it seems my original reasoning isn't showing well and the drive-by comments are piling up.

Afaict there are 3 ways to deal with the default value of length:

• stick length=16*1024 in all three function signatures for copyfileobj, copy, and copy2

  • this means any time the default needs to be changed it will be done in 3 places
  • this is the least amount of lines of code

• keep it the way it is (i.e. copy and copy2 define length=None in their signatures and one function, copyfileobj, has the current if length is None: check)

  • one more line of code then the previous solution
  • have to change the length in one place in the future
  • length is an explicit default argument in copy and copy2

• make copy and copy2 take **kwargs and pass them through to the internal call to copyfileobj

  • same lines of code as the first solution
  • doesn't explicitly document length in each of the the copy, copy2 signatures
  • one place to change the default value in the future

If one of these solutions is preferred over the others after acknowledging the trade-offs I'm fine to do it, but just simply saying length=16*1024 is less code doesn't deal with how to handle copy and copy2 and the extra (or less clear) code they may contain.

@vstinner looks like the appveyor build failed due to the with open(path, 'wb' if binary else 'w') as fp: line?

@vstinner looks like the appveyor build failed due to the with open(path, 'wb' if binary else 'w') as fp: line?

ERROR: test_copy_w_different_length (test.test_shutil.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_shutil.py", line 1417, in test_copy_w_different_length
    write_file(src.name, b'x' * 100, binary=True)
  File "C:\projects\cpython\lib\test\test_shutil.py", line 60, in write_file
    with open(path, 'wb' if binary else 'w') as fp:
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\tmpj1vhtx1b'

Sorry, I don't know why the test failed.

Looks to me like an normal permission error as python seems to not have permissions to write to the temp folder? Maybe somehow have python write to somewhere that does not require administrator permissions? It would be nice if python had an nice function that you provide the path and it checks if you have permissions to write to there to avoid having to try / except permission errors. 🤔 That function would be the perfect thing to fix this test on Windows.

Ok so besides the outstanding:

• missing news entry
• failing test

Is there anything else in the patch that needs to be addressed?

I dont think so.

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Fixed by https://bugs.python.org/issue33671