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.

Sign up for GitHub

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

Jump to bottom

Delete folder #407

Merged
merged 3 commits into from Mar 18, 2022
Merged

Delete folder #407

merged 3 commits into from Mar 18, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 18, 2022 •
edited by ghost
Loading

This patch deletes the folder before "DownloadError" and "UploadError".

Fixes #391.

Before submitting a PR to the dev branch:

  • Tests passing
  • Black formatting
  • Rebase/merge the dev branch
  • Note in the CHANGELOG

@ghost ghost mentioned this pull request Mar 18, 2022
Closed
i-oden
i-oden reviewed Mar 18, 2022
View reviewed changes
dds_cli/base.py Outdated Show resolved Hide resolved
i-oden
i-oden reviewed Mar 18, 2022
View reviewed changes
dds_cli/base.py Outdated Show resolved Hide resolved
i-oden
i-oden reviewed Mar 18, 2022
View reviewed changes
dds_cli/data_getter.py Outdated Show resolved Hide resolved
i-oden
i-oden reviewed Mar 18, 2022
View reviewed changes
dds_cli/data_putter.py Outdated Show resolved Hide resolved
i-oden
i-oden requested changes Mar 18, 2022
View reviewed changes
Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

I think it looks good and it works as I'd expect. Just a few changes though, there are a couple of lines you don't need

Zishan Mirza added 2 commits March 18, 2022 12:34
Delete folder
2ed5a71 
This patch deletes the folder before "DownloadError" and "UploadError".

Signed-off-by: Zishan Mirza <[email protected]>
Merge branch 'dev' into delete_folder
c573873 
Signed-off-by: Zishan Mirza <[email protected]>
@i-oden
Copy link
Member

i-oden commented Mar 18, 2022 •
edited
Loading

@zishanmirza Regarding the new module in the requirements.txt. As you mentioned in the chat we should have a requirements-test.txt file - we should have the same structure in this repo as in the dds_web.

  1. Create add a new file requirements-test.txt within the tests/ folder
  2. Move pytest and pyfakefs from the requirements.txt to the tests/requirements-test.txt
  3. Change this line to pip install -r tests/requirements-test.txt (I believe this should work but try it.):
    https://github.com/zishanmirza/dds_cli/blob/c573873081edeeec315d052d67ec80845bb0215c/.github/workflows/python-app.yml#L27

Delete folder
ca0e61c 
Moved "pytest" and "pyfakefs" from "requirements.txt" to
"tests/requirements-test.txt" and updated "python-app.yml".

Signed-off-by: Zishan Mirza <[email protected]>
@ghost
Copy link
Author

ghost commented Mar 18, 2022

I have moved pytest and pyfakefs from requirements.txt to tests/requirements-test.txt and updated python-app.yml.

@i-oden i-oden assigned ghost Mar 18, 2022
@i-oden i-oden added the should have Important but not vital label Mar 18, 2022
@i-oden
Copy link
Member

i-oden commented Mar 18, 2022

Looks good!

@i-oden i-oden merged commit 59175bd into ScilifelabDataCentre:dev Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
should have Important but not vital
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't create destination directory if there's no data to download / upload
1 participant
@i-oden