-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Misc FileBrowser features #1392
Conversation
@@ -100,9 +100,9 @@ def test_job_files(self): | |||
self.assertTrue(file in dir(job.files)) | |||
output_file_path = os.path.abspath(os.path.join(__file__, "..", "test_executablecontainer", "job_output_files_hdf5", "job_output_files", "error.out")) | |||
if os.name != "nt": | |||
self.assertEqual(job.files.error_out, output_file_path) | |||
self.assertEqual(str(job.files.error_out), output_file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it very confusing for the user. For example when we take a look at the NFDI example we want to be able to access the path of the files directly: https://github.com/BAMresearch/NFDI4IngScientificWorkflowRequirements/blob/main/exemplary_workflow/pyiron/workflow.py#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can fix this by supporting the File
object as input for restart_files and convert it to a string there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input_file_lst=[gmsh.files.square_msh.path]
would be an option. I don't think it would be very confusing, just extra typing. I've tried to look through the places where restart_file_lst/dict
are used and we could force a conversion there somewhere, but I'd be afraid I'll miss places. I'll revert the commits that remove it sub class if you don't see an easy path in between as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I provided a suggestion in #1397
Can do once were back from lunch. |
Allows something like ``` file = File('/some/path') for line in file: ... ```
This polutes the tab completion a lot.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
@pmrv Is there any other pull request you would like to merge in |
No, all fine with me. |
@jan-janssen I've removed the str sub classing again, because it pollutes the tab completion a bit and didn't seem to be used anywhere anymore, but I don't have too strong feelings.