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

Adding new argument to generate_dynamic: tmp_dir_path and tmp_file_name #125

Open
wants to merge 4 commits into
base: kinetic-devel
Choose a base branch
from

Conversation

jimwelagu
Copy link

@jimwelagu jimwelagu commented Jun 5, 2020

For Issue: #124

src/genpy/dynamic.py Outdated Show resolved Hide resolved
src/genpy/dynamic.py Outdated Show resolved Hide resolved
tmp_file.file.close()
if tmp_dir_path:
# Create a temporary directory with specified path
tmp_dir = os.mkdir(tmp_dir_path)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably handle the case that the directory already exists.

Copy link
Author

Choose a reason for hiding this comment

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

My thought process was that if an output_file was given, it would overwrite the file if it exists. However, if the file does not exist, it would create a file at the given output_file path.

Copy link
Member

Choose a reason for hiding this comment

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

By just removing this line it doesn't handle the case anymore where the directory doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

By just removing this line it doesn't handle the case anymore where the directory doesn't exist.

@jimwelagu This comment is still pending.

Copy link
Member

Choose a reason for hiding this comment

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

@jimwelagu Friendly ping.

src/genpy/dynamic.py Outdated Show resolved Hide resolved
@dirk-thomas
Copy link
Member

@jimwelagu Friendly ping.

@jimwelagu
Copy link
Author

@jimwelagu Friendly ping.

Made some changes per your requests.

@jimwelagu
Copy link
Author

@dirk-thomas Friendly ping.

@dirk-thomas
Copy link
Member

@jimwelagu Are you still interested in this PR and willing to address the above pending feedback?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants