Skip to content

feat: change makedir implementation to be os agnostic#456

Merged
jenhagg merged 2 commits intodevelopfrom
jon/makedir
Apr 21, 2021
Merged

feat: change makedir implementation to be os agnostic#456
jenhagg merged 2 commits intodevelopfrom
jon/makedir

Conversation

@jenhagg
Copy link
Copy Markdown
Collaborator

@jenhagg jenhagg commented Apr 21, 2021

Purpose

Breaking the refactoring to support native windows users into smaller chunks. This one simply defines the DataAccess.makedir method separately for local/ssh subclasses.

What the code is doing

Change mkdir command to os.makedirs for local calls.

Testing

Ran prepare_simulation_input() on new scenario in fresh container. The code used for the server is the same as before.

Time estimate

5 min

@jenhagg jenhagg requested a review from rouille April 21, 2021 21:56
@jenhagg jenhagg self-assigned this Apr 21, 2021
@jenhagg jenhagg requested a review from ahurli April 21, 2021 21:57
@jenhagg jenhagg added this to the Let the Sun Shine milestone Apr 21, 2021
_, _, stderr = self._data_access.makedir(self.TMP_DIR)
if len(stderr.readlines()) != 0:
raise IOError("Failed to create %s on server" % self.TMP_DIR)
self._data_access.makedir(self.REL_TMP_DIR)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This previously worked since one path was a prefix of the other one. Just changed so it's consistent with the assumption in makedir that we are using a relative path.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need to keep the TMP_DIR attribute defined in the constructor?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, it's not used anywhere now

@jenhagg jenhagg linked an issue Apr 21, 2021 that may be closed by this pull request
5 tasks
@jenhagg jenhagg removed a link to an issue Apr 21, 2021
5 tasks
Copy link
Copy Markdown
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

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

Looks good

@jenhagg jenhagg merged commit 57bddfa into develop Apr 21, 2021
@jenhagg jenhagg deleted the jon/makedir branch April 21, 2021 22:22
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.

2 participants