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 http file download functionality #33

Merged
merged 10 commits into from
Apr 2, 2022
Merged

Conversation

Milkshak3s
Copy link
Collaborator

What type of PR is this?

Implements the download function for the implant.

This is a solution for #19

/kind feature

What this PR does / why we need it:

Implements the download function for the Eldritch implant

Which issue(s) this PR fixes:

Fixes #19

@Milkshak3s Milkshak3s added the feature New feature or request label Mar 30, 2022
@Milkshak3s
Copy link
Collaborator Author

Milkshak3s commented Mar 30, 2022

Tests failing in GH but I can't see why.
EDIT: Tests pass locally*

Documented in #37

@hulto
Copy link
Collaborator

hulto commented Mar 30, 2022

Tests failing in GH but I can't see why.

EDIT: Tests pass locally*

Documented in #37

Probably a billing issue have to wait for the new month for CI to start working.

@hulto
Copy link
Collaborator

hulto commented Mar 30, 2022

Please add a description to:
docs / _docs / user_guide.me

Milkshak3s and others added 5 commits April 1, 2022 13:22
moved from _dst _uri -> dst uri to keep in line with used var naming scheme.
Copy link
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

LGTM. 🚀

Copy link
Collaborator

@hulto hulto 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 😎. Just a little more clarity in the docs on how existing files are handled.

@@ -27,7 +27,7 @@ The <b>file.copy</b> copies a file from src path to dst path. If dst path doesn'
### file.download
`file.download(uri: str, dst: str) -> None`

The <b>file.download</b> method is very cool, and will be even cooler when Nick documents it.
The <b>file.download</b> method downloads a file at the URI specified in `uri` to the path specified in `dst`. This currently only supports `http` & `https` protocols.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a description of what will happen if the dst file already exists.

Copy link
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

Magical! 🧙‍♂️

@Milkshak3s Milkshak3s merged commit decef24 into main Apr 2, 2022
@Milkshak3s Milkshak3s deleted the milkshak3s-eld-file-download branch April 2, 2022 14:52
KCarretto pushed a commit that referenced this pull request Feb 1, 2024
 
Add http file download functionality (#33)

* Add http file download functionality

* Update download_impl.rs

* Remove my print testing whoops

* Add docs

* Updated vars

moved from _dst _uri -> dst uri to keep in line with used var naming scheme.

* Fixed one more ^

* Updated var naming scheme.

* Update eldritch.md

Co-authored-by: Hulto <hulto@hul.to>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Eldritch: Implement file.Download
2 participants