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 default option of downloading files from given URL, instead of fixed github one #247

Merged
merged 7 commits into from
Sep 1, 2023

Conversation

Sheshuk
Copy link
Contributor

@Sheshuk Sheshuk commented May 20, 2023

Following my suggestion on making a separate repository for the data files, I would like to add the capability of downloading the files from any given url.

URLs can be formatted with any parameters, so I implemented the same github downloading in a new format for illustration:

Copy link
Member

@JostMigenda JostMigenda left a comment

Choose a reason for hiding this comment

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

Some minor cleanup; otherwise this looks fine. Once we have a separate models directory and have tested that this works, I think we can remove the from_github function, which is just a duplicate of the default case, right?

python/snewpy/_model_downloader.py Outdated Show resolved Hide resolved
@Sheshuk
Copy link
Contributor Author

Sheshuk commented Jun 3, 2023

I removed the from_github, cleaned up the docstrings, and also added a default parameter snewpy_version which can be put to the URL template. So it will always point to the current version.

@JostMigenda JostMigenda self-requested a review July 19, 2023 09:11
Copy link
Member

@JostMigenda JostMigenda left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Sheshuk!

@JostMigenda JostMigenda merged commit feb3a6c into main Sep 1, 2023
8 checks passed
@JostMigenda JostMigenda deleted the Sheshuk/models_download_from_url branch September 1, 2023 09:31
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.

None yet

2 participants