[Fix #438] Disallow empty project names#472
[Fix #438] Disallow empty project names#472chandrikarj wants to merge 3 commits intoneuroinformatics-unit:mainfrom
Conversation
| "Project name cannot be empty. Please enter a valid name.", | ||
| severity="error", | ||
| ) | ||
| print( |
There was a problem hiding this comment.
I think instead of using a print statement, it would be more helpful to display this message in a dialog box. That way, users are immediately informed about the issue without needing to check logs.
There was a problem hiding this comment.
Hi @parharti, well your suggestion makes sense, and, the users don't need to check logs and can already see the statement in a dialog box. I had added the print statement to debug the same. Should you wish to, you can see it in the video linked under demonstration of the changes.
JoeZiminski
left a comment
JoeZiminski
left a comment
There was a problem hiding this comment.
Hey @chandrikarj thanks a lot for this! This implementation is working very nicely. I think however we can make this change further up the datashuttle call chain, and it will show the error both in the TUI and in the python API.
Under the hood, when the TUI is instantiated the python API is used. In code this would look like:
from datashuttle import DataShuttle
project = DataShuttle(")
This instantiates this class and sets the project name here.
If we place a check here, it will raise and error both in the python API and in the TUI for free. I just realized there is already a function here called _error_on_base_project_name to check the name does not contain special characters, but it misses the empty case. You could try adding the case here, and I think it will be caught in the python API and TUI.
Let me know if you have any questions!
| "Project name cannot be empty. Please enter a valid name.", | ||
| severity="error", | ||
| ) | ||
| print( |
There was a problem hiding this comment.
It is okay to remove these print statements now, these are useful in development but if the error is already notified through the TUI these can be removed
| project_name = self.interface.project.cfg.project_name | ||
| # Add validation before proceeding | ||
| if not project_name.strip(): | ||
| self.notify( |
There was a problem hiding this comment.
Nice! This is a cool textual feature, I think we could move towards using it one day. Currently we use modal dialog boxes to display errors, the class is here and called for example here. It achieves the same idea but is just more aligned with how the project currently displays errors for consistency. However, see my comment below I think we can get this for free with a change in a slightly different place in the codebase.
| """ | ||
| project_name = self.query_one("#configs_name_input").value | ||
| # Add validation before proceeding | ||
| if not project_name.strip(): |
There was a problem hiding this comment.
Just as a general tip, if you have a piece of code you are using in one place, you can create a function and place the code there, then call it from multiple places. The benefit of this is that if you need to change the code in future, you only have to do it in one place.
|
Thanks for your comments, working on this! |
|
Hey @chandrikarj hope everything is going well, further to our chat would you be be happy for me to to take over this PR (I just checked and I am able to push to the branch). Cheers! |
|
Hey @chandrikarj I will close this for now, thanks again!" |
3 participants
Description
Fixes issue #438 : Disallow empty file name in TUI project setup
What is this PR
Why is this PR needed?
Currently, the TUI allows users to create a project with an empty name, which can lead to unexpected behavior.
This PR introduces a validation step to ensure that project names cannot be empty. If an empty name is detected, an error message is displayed, preventing project creation.
What does this PR do?
Demonstration of the changes:
https://github.com/user-attachments/assets/6b3fa0ec-e25b-4fae-a423-67d59b126062
References
Fix for issue #438
How has this PR been tested?
Does this PR require an update to the documentation?
Yes, minor update required to specify project name validation in the documentation.
Checklist: