Skip to content

Refine overwrite options#663

Merged
JoeZiminski merged 3 commits intoneuroinformatics-unit:imrpove_transfer_overwrite_optionsfrom
Sushma-1706:refine-overwrite-options
Feb 27, 2026
Merged

Refine overwrite options#663
JoeZiminski merged 3 commits intoneuroinformatics-unit:imrpove_transfer_overwrite_optionsfrom
Sushma-1706:refine-overwrite-options

Conversation

@Sushma-1706
Copy link
Contributor

Description

What is this PR?

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
The existing always overwrite option was misleading because it did not
actually force overwriting files. By default, rclone only copies files when
timestamps or checksums differ, which caused confusion for users.

This PR clarifies the overwrite behavior and aligns it with rclone semantics.

What does this PR do?
This PR refines overwrite behavior for transfers by:

  1. Adding a new if_different option that uses rclone’s default behavior
  2. Updating always to use --ignore-times so it truly overwrites files
  3. Leaving never and if_source_newer unchanged
  4. Making overwrite behavior explicit and predictable

References

Closes #624

How has this PR been tested?

This change affects argument construction only.
The logic was verified by inspecting the generated rclone arguments and ensuring
they match the expected rclone flags.

No runtime behavior or external dependencies were modified.

Is this a breaking change?

No.
This change clarifies behavior but does not remove or rename any existing options.

Does this PR require an update to the documentation?

Yes — documentation updated in transfer-data.md.

Checklist

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@JoeZiminski
Copy link
Member

Hi @Sushma-1706 thanks for this! This looks good to me, I'm at a conference this week but will take a proper look next week. We will have to extend the tests a bit for this, but in the meantime I think this can be extended to the terminal-user interface.

If you type 'datashuttle launch' in a terminal (in your environment where datashuttle is installed) it will start the TUI. You can see on the docs how to set up a project, but on the 'transfer' tab we have a dropdown that allows the user to select the transfer method. This new option could be added there. Let me know if you have any questions!

@Sushma-1706
Copy link
Contributor Author

Thanks @JoeZiminski, glad to hear it looks good!
That makes sense — I agree the new overwrite option would be useful in the TUI as well.
I’m happy to take that on as a follow-up change once this PR is reviewed/merged, so we can keep this one focused.
Looking forward to your review next week — thanks!

@JoeZiminski
Copy link
Member

Hi @Sushma-1706 apologies for the delay, I thought I had responded here! I had a look through the code it all looks good, I think we can move on to testing. We will also include the TUI implementation in this PR. Although it makes PRs larger, it is easier to manage when a single PR contains the implementation for both the Python API and TUI. This way ensures that the Python API and TUI do not come out of alignment.

We can extend the testing framework for these new cases. You can see here there is a simple test here that just checks the options are passed through successfully. We also have functional tests here (this and the below 3 tests). These will need to be updated to handle the next naming conventions. Also, if you see any of the new cases are not tested by these functions, please let me know and we can add one. Thanks!

For testing the TUI, we can think about this after the feature has been implemented for the TUI. Again please let me know if you have any questions!

@JoeZiminski JoeZiminski changed the base branch from main to imrpove_transfer_overwrite_options February 27, 2026 11:08
@JoeZiminski
Copy link
Member

Hi @Sushma-1706 thanks for this, this has become high priority and so I will merge this branch into a new working branch. This will ensure your commits and contributions are maintained. I will extend this to the TUI and add tests. Thanks!

@JoeZiminski JoeZiminski merged commit 4d42d2b into neuroinformatics-unit:imrpove_transfer_overwrite_options Feb 27, 2026
15 checks passed
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.

Refine the different overwrite transfer arguments

2 participants