Skip to content

feat: os agnostic rm functionality#461

Merged
jenhagg merged 4 commits intodevelopfrom
jon/remove
Apr 26, 2021
Merged

feat: os agnostic rm functionality#461
jenhagg merged 4 commits intodevelopfrom
jon/remove

Conversation

@jenhagg
Copy link
Copy Markdown
Collaborator

@jenhagg jenhagg commented Apr 22, 2021

Purpose

Another small chunk to support windows natively. This one splits the remove method so that the one in LocalDataAccess is os independent.

What the code is doing

  • Add confirmation prompt before running rm command on server, and before deleting local glob matches
  • Move the error check on stderr inside the SSHDataAccess implementation, since there is no analog for local version (return types have to match)
  • Remove force parameter and hard code the resulting -f option since we always pass it to the server, and it's not needed locally
  • Change value passed to recursive in move.py so it matches what we do in delete.py
  • Rename test file to match module under test

Testing

The main thing to test is the new glob used in the local implementation. I did this manually by creating some nested folders and checking the files matched. Also tested with the confirmation prompt using the server based container setup.

Time estimate

15 min

@jenhagg jenhagg self-assigned this Apr 22, 2021
@jenhagg jenhagg added this to the Let the Sun Shine milestone Apr 22, 2021
Comment thread powersimdata/data_access/data_access.py Outdated
"""
if recursive:
target = os.path.join(target, "**")
files = glob.glob(target, recursive=recursive)
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.

According to the documentation, it seems glob is used for Unix shell only?

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.

I believe "unix style" is just a reference to the origin. There are separate sections in the docs for windows and unix specific features. Will do a test to confirm though.

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.

Looks like there could be some inconsistencies (see this post) but it should work for our current use case.

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.

Thanks for looking into that. I think it should be fine as long as it works for our current use case. As said, we can always revisit it in the future if our usage goes beyond the available scope.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this fails, it seems like we can use regex workarounds. I don't think we use any [ or ] characters within our data directories, so hopefully any of those characters in parent directories won't be a problem.

Copy link
Copy Markdown
Collaborator

@BainanXia BainanXia left a comment

Choose a reason for hiding this comment

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

Not sure how to test it throughout. But some native tests locally on my mac work as expected.

Comment thread powersimdata/data_access/data_access.py Outdated
Comment thread powersimdata/scenario/delete.py Outdated
Comment thread powersimdata/scenario/delete.py Outdated
Comment thread powersimdata/scenario/move.py
Comment thread powersimdata/scenario/move.py
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 824f89e into develop Apr 26, 2021
@jenhagg jenhagg deleted the jon/remove branch April 26, 2021 20:46
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.

5 participants