Skip to content

Add progress option#15

Merged
woelper merged 8 commits into
masterfrom
copy_progress
Mar 24, 2026
Merged

Add progress option#15
woelper merged 8 commits into
masterfrom
copy_progress

Conversation

@woelper

@woelper woelper commented Mar 14, 2026

Copy link
Copy Markdown
Owner

This PR adds a progress option which can be specified by building the options .with_progress().
Closes #14 .

@woelper woelper mentioned this pull request Mar 14, 2026
@woelper woelper changed the title Cleanup and docs Add progress option Mar 14, 2026
@michaeljones

Copy link
Copy Markdown

Hi, thanks again for looking into this. I've just tried it for our use case and the Send + Sync requirement on the progress callback makes things a bit harder. I think I might be able to manage the situation with some Arc<Mutex<..>> usage but I'm curious if Send + Sync is required in this situation?

I think it is ok to expect the CopyBuilder to be constructed and run in the same thread? But I might be overlooking something. I use Rust quite a lot but I don't have the best knowledge of these things.

My issue at the moment is that I'm using the with_progress callback to log progress but the logging set up we have requires calling a method on an 'Output' value and that value isn't Send + Sync.

It is very much up to you how you'd like to handle it. I'm just curious to understand it better. Thanks again.

@woelper

woelper commented Mar 17, 2026

Copy link
Copy Markdown
Owner Author

Hi, thanks again for looking into this. I've just tried it for our use case and the Send + Sync requirement on the progress callback makes things a bit harder. I think I might be able to manage the situation with some Arc<Mutex<..>> usage but I'm curious if Send + Sync is required in this situation?

I think it is ok to expect the CopyBuilder to be constructed and run in the same thread? But I might be overlooking something. I use Rust quite a lot but I don't have the best knowledge of these things.

My issue at the moment is that I'm using the with_progress callback to log progress but the logging set up we have requires calling a method on an 'Output' value and that value isn't Send + Sync.

It is very much up to you how you'd like to handle it. I'm just curious to understand it better. Thanks again.

Hi Michael! Thank you for getting back so quickly!

Send and Sync is not a hard requirement - my thought was that later we might need this when parallel copying should be possible. Maybe it is better and simpler to remove Send and Sync and actually add it once parallelism is needed - or just not support progress in that case. What do you think?

@woelper

woelper commented Mar 21, 2026

Copy link
Copy Markdown
Owner Author

The send and sync is now removed and I have added some more tweaks to the tests.

@michaeljones

Copy link
Copy Markdown

Hi, thank you for the updates. I've tested them for our project and it works well without needing Arc & Mutex on my end.

I support you doing in whatever direction you think best. Given that dircpy is a reasonably small crate with a clear API then I think you could introduce Send + Sync only when it is really needed and do a major version bump if it is a breaking change but I think it is easier for someone on the outside to suggest that and you might have different feelings as the maintainer.

Thanks again for engaging with the idea and for such a tidy API.

@woelper

woelper commented Mar 23, 2026

Copy link
Copy Markdown
Owner Author

Hi, thank you for the updates. I've tested them for our project and it works well without needing Arc & Mutex on my end.

I support you doing in whatever direction you think best. Given that dircpy is a reasonably small crate with a clear API then I think you could introduce Send + Sync only when it is really needed and do a major version bump if it is a breaking change but I think it is easier for someone on the outside to suggest that and you might have different feelings as the maintainer.

Thanks again for engaging with the idea and for such a tidy API.

Thank you for the kind words, and I think it makes sense. I'll go with the simpler approach and do send/sync when it is actually needed.

@woelper
woelper merged commit cfee8ba into master Mar 24, 2026
6 checks passed
@woelper
woelper deleted the copy_progress branch April 15, 2026 18:38
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.

Progress reporting

2 participants