Skip to content

Conversation

@NobleCoder69
Copy link

@NobleCoder69 NobleCoder69 commented Oct 18, 2025

Fix transfer if fields are left blank. (Final formatting fix)

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)


Description

This PR fixes the issue where the TUI transfer process failed when the subject or session fields were left blank.
It also corrects type hints in the affected functions to use list[str] instead of generic List types.
Local verification of Sphinx docs was blocked due to environment-related issues, but all code changes were tested locally.


What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Previously, leaving the subject or session fields empty in the transfer interface would cause the operation to fail.
This fix ensures that transfers proceed successfully even when these fields are blank, improving usability and robustness.


What does this PR do?

  • Sets default values for subject and session when left blank.
  • Corrects type hints to use list[str].
  • Finalizes code formatting for consistency with pre-commit.

References

Fixes: #619


How has this PR been tested?

  • Verified locally by running pytest and manually testing the TUI transfer workflow.
  • Confirmed that defaulting subject and session no longer interrupts transfer execution.
  • Ran pre-commit successfully for code style and static checks.
  • Unable to fully reproduce the Sphinx documentation build locally due to environment issues.

Is this a breaking change?

No, this change is backward-compatible.
It only modifies behavior when the subject or session fields are left blank, and does not affect existing workflows.


Does this PR require an update to the documentation?

No documentation updates are required, as this change only affects default handling in the TUI transfer behavior.


Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (if applicable)
  • The documentation has been updated to reflect any changes (not required for this PR)
  • The code has been formatted with pre-commit

…l' in TUI custom transfer if fields are left blank. (Final formatting fix)
@NobleCoder69 NobleCoder69 marked this pull request as ready for review October 18, 2025 21:49
@adamltyson
Copy link
Member

Hi @NobleCoder69 could you make sure to fill out the PR template?

@NobleCoder69
Copy link
Author

Hi @adamltyson,
I’ve filled out the PR template and verified the TUI transfer fix locally.
The Sphinx build failure looks environment-related (it also occurred in previous PRs).
The code changes themselves pass local tests and pre-commit checks.

Ready for review when convenient. 👍

@JoeZiminski
Copy link
Member

Thanks @NobleCoder69, I'm not sure I can see any changes to the typing here although mentioned in the PR body. Nonetheless this indeed sets None default to all, which is a solution to #619, but I think it would be better targetted to the TUI. To do this, you can take a look at this code.

It is quite good that you are working on this issue because the most user friendly way to handle this is not clear, and requires some thought. If you could generate a test project and play around with the transfer, and let me know what you think is the best 'default' for leaving this boxes empty. The current default of passing through as None is very confusing, we could throw an error, or pre-fill to "all", and maybe there are other solutions.

@NobleCoder69
Copy link
Author

Thanks a lot for the feedback! I see what you mean about handling this in the TUI instead. I’ll go through the code you mentioned and create a small test project to experiment with different defaults. I’ll try both throwing an error and pre-filling “all” to see what feels more intuitive, then share my thoughts here.

@JoeZiminski
Copy link
Member

Hi @NobleCoder69, are you still interested in working on this issue?

@NobleCoder69
Copy link
Author

Hi all — I’m still actively working on this. Thanks for the feedback @JoeZiminski.

Plan

  • Apply the defaulting behavior in the TUI layer only (blank → "all")
  • Add helper text in the prompt to explain that leaving it blank selects all
  • Add unit tests for blank and comma-separated inputs
  • Keep backend behavior unchanged

I’ll push a follow-up commit with tests and update the PR description accordingly.

If reviewers prefer a different token (e.g., None or "") I can adjust the translation step in the TUI.

Also: I noticed a CI failure on macOS Python 3.9 — if it still fails after my update, I’ll check logs and ask for guidance if needed. It may be unrelated / flaky.

Thanks!

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.

Do not all blank sub or ses in the custom transfer TUI

3 participants