Skip to content
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

Add output files class #1311

Merged
merged 50 commits into from
Feb 8, 2024
Merged

Add output files class #1311

merged 50 commits into from
Feb 8, 2024

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Feb 4, 2024

In comparison to the previous work in #969 this pull request only lists the file paths for the files in the working directory.

Alternatively, we could also consider combining both pull requests.

@jan-janssen jan-janssen marked this pull request as draft February 4, 2024 10:13
@jan-janssen jan-janssen requested a review from pmrv February 5, 2024 19:44
@jan-janssen jan-janssen marked this pull request as ready for review February 5, 2024 19:46
@jan-janssen jan-janssen marked this pull request as draft February 6, 2024 15:33
@jan-janssen jan-janssen added format_black reformat the code using the black standard and removed format_black reformat the code using the black standard labels Feb 6, 2024
@jan-janssen
Copy link
Member Author

jan-janssen commented Feb 6, 2024

@pmrv I merged your previous branch into this one and modified it to fulfil your requirements (based on the tests) as well as mine. From my perspective the pull request is now ready to be reviewed.

@jan-janssen jan-janssen added format_black reformat the code using the black standard and removed format_black reformat the code using the black standard labels Feb 7, 2024
@jan-janssen jan-janssen marked this pull request as ready for review February 7, 2024 08:02
Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some small nits, but otherwise lgtm.

pyiron_base/jobs/job/core.py Outdated Show resolved Hide resolved
pyiron_base/jobs/job/extension/files.py Outdated Show resolved Hide resolved
raise AttributeError(item) from None


class File(str):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class File(str):
class File:
__slots__ = ("_path",)
def __init__(self, path: str):
"""
Wrap a file.
Args:
path (str): path to file
"""
self._path = path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We not actually using any string functionality here and I'd rather not let people accidentally perform string operations on this object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I would like to keep the string part in, is that then I can use os.path.join() on the object, this is what I need to access the path.

Comment on lines +120 to +121
working_directory=os.path.dirname(self),
file_name=os.path.basename(self),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
working_directory=os.path.dirname(self),
file_name=os.path.basename(self),
working_directory=os.path.dirname(self._path),
file_name=os.path.basename(self._path),

See above.

pyiron_base/jobs/job/extension/files.py Outdated Show resolved Hide resolved
@jan-janssen jan-janssen merged commit 13f2a30 into main Feb 8, 2024
24 of 25 checks passed
@jan-janssen jan-janssen deleted the output_files branch February 8, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants