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

standard kwargs not supported in copytree #249

Open
brno32 opened this issue Nov 20, 2023 · 5 comments
Open

standard kwargs not supported in copytree #249

brno32 opened this issue Nov 20, 2023 · 5 comments

Comments

@brno32
Copy link
Contributor

brno32 commented Nov 20, 2023

Hello,

I tried to specify buffering and share_access in smbclient.shutil.copytree like so:

smbclient.shutil.copytree(src, dist, buffering=SIZE, share_access="r")

and it yields the following error

TypeError: get_smb_tree() got an unexpected argument 'buffering'

Looking at the source code, I can see why. Inside of scandir, which copytree calls internally, these kwargs get forwarded to get_smb_tree().

Can we add some additional logic to pop these kwargs when specified inside of copytree, but use their values where appropriate? (such as in copyfile)

@jborean93
Copy link
Owner

Hmm I can see buffering being something we may want on there as at least it's own option to use when copying files but I'm unsure whether share_access should be exposed and whether we should just always be opening with a read share access. I can't think of a reason why that shouldn't always be there for the source file.

@brno32
Copy link
Contributor Author

brno32 commented Nov 24, 2023

If the underlying function that copies the files exposes share_access I think it'd be nice to have control over it from the copytree function that wraps it

@jborean93
Copy link
Owner

I’m just saying what would the purpose be for it. I can understand doing read share access by default but allowing write or delete means the file could be changed as it is being read from. This would also only apply to the source file, the dest file would most likely not have have share flags because the contents are being changed.

I’m just not sure there are any benefits to allow a user defined share_access here aside from changing the default on the source file.

@brno32
Copy link
Contributor Author

brno32 commented Nov 24, 2023

fair enough. I could make a PR that:

  • exposes buffering as an argument to copytree
  • sets share_access="r" by default inside of copytree

@jborean93
Copy link
Owner

Sounds fair, keep in mind the copytree function is quite complicated. There are two different types of copy modes used one for when the files are on the same share which does a server side copy and another when the files are on different locations which falls back to shutil.copyfileobj. I would probably investigate the reasons why you want to provide an explicit buffering value here rather than expose a new kwarg that may or may not be used depending on the src and dst paths.

Setting the share_access would be done on the src_open but only when src_open is our SMB open_file implementation. Setting it on the dest open wouldn't really make much sense there.

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

No branches or pull requests

2 participants