Improve rclone transfer output detail. #623

JoeZiminski · 2025-10-20T17:26:25Z

This PR improves the detail on the rclone copy log output and highlights all errors in the transfer message.

In the python API, a short summary is printed after the logs. In the TUI, a 'warning' is given if nothing as transferred:

and errors are show:

These are generatede from directly parsing the rclone logs. I am waiting for somone from RClone to check this approach here, I will follow up on / improve the request if Id ont hear back soon.

…chinery.

Copilot

Pull Request Overview

This PR enhances rclone transfer output logging by parsing JSON-formatted logs to extract and display error details and transfer statistics. The implementation adds error tracking throughout the transfer pipeline, from the core rclone wrapper to the TUI and API, enabling users to see specific file errors and warnings when no files are transferred.

Key Changes:

Added TransferErrors TypedDict to track file-level errors and transfer completion status
Implemented JSON log parsing in rclone.py to extract errors and statistics from rclone output
Modified all transfer functions to return error information for display in both API and TUI contexts

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
datashuttle/utils/custom_types.py	Added `TransferErrors` TypedDict to define the structure for error tracking
datashuttle/utils/rclone.py	Implemented JSON log parsing, error extraction, and formatting functions for rclone output
datashuttle/utils/data_transfer.py	Modified `TransferData` class to return errors from transfer operations
datashuttle/datashuttle_class.py	Updated all transfer methods to return `TransferErrors` and display error summaries
datashuttle/tui/interface.py	Updated TUI interface transfer methods to pass errors to UI components
datashuttle/tui/screens/modal_dialogs.py	Enhanced MessageBox to support variable sizing and display detailed error messages
datashuttle/tui/css/tui_menu.tcss	Moved messagebox sizing from CSS to Python for dynamic adjustment
datashuttle/utils/ssh.py	Fixed missing newline character in error message
pyproject.toml	Added `filelock` dependency for testing file locking scenarios
tests/test_utils.py	Added `lock_a_file` utility function for simulating file locks in tests
tests/tests_tui/test_tui_transfer.py	Added comprehensive test for error display in TUI pop-ups
tests/tests_transfers/local_filesystem/test_transfer.py	Added test to verify errors variable is properly returned from all transfer functions
tests/tests_integration/test_logging.py	Added test to verify errors are caught, logged, and displayed correctly
tests/tests_tui/tui_base.py	Added optional parameter to control messagebox closing in transfer helper

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-20T15:15:26Z

tests/tests_integration/test_logging.py

+
+        test_utils.delete_log_files(project.cfg.logging_path)
+
+        test_utils.delete_log_files(project.cfg.logging_path)


Duplicate call to delete_log_files. This line is redundant as the same call is made on line 518.

Suggested change

test_utils.delete_log_files(project.cfg.logging_path)

Copilot · 2025-11-20T15:15:26Z

tests/test_utils.py

+    """"""
+


Missing docstring. This function should have a docstring that describes its purpose, parameters, and return value.

Suggested change

""""""

"""

Lock a file by writing to it continuously for a specified duration in a separate thread.

Parameters

----------

file_path : str or Path

The path to the file to be locked.

duration : int, optional

The duration in seconds to keep the file locked (default is 5).

Returns

-------

threading.Thread

The thread object that is writing to the file.

"""

Copilot · 2025-11-20T15:15:27Z

datashuttle/utils/rclone.py

+    utils.log_and_message(message)
+
+
+def log_rclone_copy_errors_api(errors):


Missing type hints. The function should include type hints for the errors parameter (should be TransferErrors) to improve type safety and code clarity.

Suggested change

def log_rclone_copy_errors_api(errors):

def log_rclone_copy_errors_api(errors: "TransferErrors"):

Copilot · 2025-11-20T15:15:27Z

datashuttle/utils/rclone.py

+    utils.log_and_message(message, use_rich=True)
+
+
+def parse_rclone_copy_output(top_level_folder, output):


Missing type hints. The function should include type hints for the top_level_folder parameter (should be TopLevelFolder | None) and the output parameter (should be CompletedProcess) for better type safety.

Suggested change

def parse_rclone_copy_output(top_level_folder, output):

def parse_rclone_copy_output(

top_level_folder: TopLevelFolder | None, output: CompletedProcess

) -> tuple[str, str, dict]:

Copilot · 2025-11-20T15:15:27Z

datashuttle/utils/data_transfer.py


        self.check_input_arguments()

+    def run(self):


Missing return type hint. The run method should have a return type hint of TransferErrors. This would require importing TransferErrors from datashuttle.utils.custom_types.

Copilot · 2025-11-20T15:15:29Z

datashuttle/utils/rclone.py

+        "nothing_was_transferred_rawdata": err_errors[
+            "nothing_was_transferred_rawdata"
+        ],
+        "nothing_was_transferred_derivatives": err_errors[
+            "nothing_was_transferred_derivatives"
+        ],


Potential data loss when combining errors. When combining errors from stdout and stderr, only the nothing_was_transferred flags from err_errors are used. If out_errors also contains relevant information (e.g., nothing_was_transferred_rawdata or nothing_was_transferred_derivatives is set), it will be lost. Consider merging these flags with appropriate logic (e.g., using OR logic or choosing the non-None value).

Suggested change

"nothing_was_transferred_rawdata": err_errors[

"nothing_was_transferred_rawdata"

],

"nothing_was_transferred_derivatives": err_errors[

"nothing_was_transferred_derivatives"

],

"nothing_was_transferred_rawdata": (

out_errors["nothing_was_transferred_rawdata"]

if out_errors["nothing_was_transferred_rawdata"] is not None

else err_errors["nothing_was_transferred_rawdata"]

),

"nothing_was_transferred_derivatives": (

out_errors["nothing_was_transferred_derivatives"]

if out_errors["nothing_was_transferred_derivatives"] is not None

else err_errors["nothing_was_transferred_derivatives"]

),

Copilot · 2025-11-20T15:15:29Z

datashuttle/tui/screens/modal_dialogs.py

+                if any(errors["messages"]):
+                    if errors["file_names"]:
+                        errors_message += (
+                            "\n[red]Errors detected! in files:[/red]\n"


Inconsistent capitalization in error message. The error message "Errors detected! in files:" has inconsistent capitalization - "in" should be capitalized as "In" to match the sentence structure, or the exclamation mark should be replaced with a comma.

Suggested change

"\n[red]Errors detected! in files:[/red]\n"

"\n[red]Errors detected, in files:[/red]\n"

Copilot · 2025-11-20T15:15:29Z

tests/tests_tui/test_tui_transfer.py

+
+            # Transfer again to check the message displays indicating
+            # no files were transferred.
+            await self.run_transfer(pilot, "upload")


Redundant transfer operation. The transfer is run twice consecutively (lines 295 and 297-299). The first call on line 295 closes the messagebox immediately, which seems unnecessary since the second call on line 297 is the one being tested. Consider removing line 295.

Suggested change

await self.run_transfer(pilot, "upload")

Copilot · 2025-11-20T15:15:29Z

datashuttle/tui/screens/modal_dialogs.py

+        height
+            The height of the messagebox.
+
+        width
+            The width of the messagebox.
+


Inconsistent parameter order in docstring. The docstring lists parameters in the order message, border_color, height, width, but the function signature has them in the order message, border_color, width, height. The docstring should match the function signature order.

Suggested change

height

The height of the messagebox.

width

The width of the messagebox.

width

The width of the messagebox.

height

The height of the messagebox.

Copilot · 2025-11-20T15:15:30Z

datashuttle/tui/screens/modal_dialogs.py

+            else:
+                self.app.show_modal_error_dialog(output)
+
+        except BaseException as e:


Except block directly handles BaseException.

Suggested change

except BaseException as e:

except Exception as e:

JoeZiminski force-pushed the extend_rclone_log_output branch 2 times, most recently from f49735b to f6434b6 Compare October 23, 2025 15:03

JoeZiminski marked this pull request as ready for review October 23, 2025 16:17

JoeZiminski linked an issue Nov 1, 2025 that may be closed by this pull request

Better indication of transfer sucess / fail #432

Open

JoeZiminski and others added 21 commits November 3, 2025 22:15

Improve rclone transfer output detail.

447b2a1

Full draft implementation for TUI and API.

9f642d7

Some tidying up.

224e3bb

Extend to critical errors.

5cb61be

Add test for errors output.

a81db02

Finalise tests.

dd0aeaa

Also show message if nothing was transferred.

15d8102

Docstrings and linting.

85f90f6

Tidy up tests, add some docstrings.

5afba61

Add a bit more documentation.

9763df8

Tidy up and fix tests.

17a51aa

Fix tests...again.

7f73b3f

Fix tests on linux.

4a562a8

Playing around windows only...

55ef242

Tests, different cases for locking file on windows vs. macos/linux.

e04c8ae

Minor reformating on error messages.

6772736

Revert worlfkow changes.

e2de477

Adjust tests to investigate failures.

3153bff

Fix path separator issues.

47a8a90

Fix newline.

826534d

Add back all tests.

bc27a76

JoeZiminski force-pushed the extend_rclone_log_output branch from e02bc41 to bc27a76 Compare November 3, 2025 22:18

JoeZiminski and others added 4 commits November 3, 2025 23:35

Try again! to fix these weird test failures.

d4834c5

Try moving to logging module to take advantage of the logging test ma…

06c77c8

…chinery.

Fix CI script.

ad76736

Revert ci changes.

5778043

JoeZiminski requested a review from Copilot November 20, 2025 15:09

Copilot started reviewing on behalf of JoeZiminski November 20, 2025 15:10 View session

Copilot finished reviewing on behalf of JoeZiminski November 20, 2025 15:13

Copilot AI reviewed Nov 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve rclone transfer output detail. #623

Improve rclone transfer output detail. #623

Uh oh!

JoeZiminski commented Oct 20, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Copilot AI Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		test_utils.delete_log_files(project.cfg.logging_path)

		test_utils.delete_log_files(project.cfg.logging_path)

-    """"""
+    """
+    Lock a file by writing to it continuously for a specified duration in a separate thread.
+    Parameters
+    ----------
+    file_path : str or Path
+        The path to the file to be locked.
+    duration : int, optional
+        The duration in seconds to keep the file locked (default is 5).
+    Returns
+    -------
+    threading.Thread
+        The thread object that is writing to the file.
+    """

		utils.log_and_message(message)


		def log_rclone_copy_errors_api(errors):

	def log_rclone_copy_errors_api(errors):
	def log_rclone_copy_errors_api(errors: "TransferErrors"):

		utils.log_and_message(message, use_rich=True)


		def parse_rclone_copy_output(top_level_folder, output):

-        "nothing_was_transferred_rawdata": err_errors[
-            "nothing_was_transferred_rawdata"
-        ],
-        "nothing_was_transferred_derivatives": err_errors[
-            "nothing_was_transferred_derivatives"
-        ],
+        "nothing_was_transferred_rawdata": (
+            out_errors["nothing_was_transferred_rawdata"]
+            if out_errors["nothing_was_transferred_rawdata"] is not None
+            else err_errors["nothing_was_transferred_rawdata"]
+        ),
+        "nothing_was_transferred_derivatives": (
+            out_errors["nothing_was_transferred_derivatives"]
+            if out_errors["nothing_was_transferred_derivatives"] is not None
+            else err_errors["nothing_was_transferred_derivatives"]
+        ),

	"\n[red]Errors detected! in files:[/red]\n"
	"\n[red]Errors detected, in files:[/red]\n"

Improve rclone transfer output detail. #623

Are you sure you want to change the base?

Improve rclone transfer output detail. #623

Uh oh!

Conversation

JoeZiminski commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JoeZiminski commented Oct 20, 2025 •

edited

Loading