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

[Fix] update data/database.py to use urllib methods. fixes #1061 #1063

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

ignatious-p
Copy link

@ignatious-p ignatious-p commented Jan 23, 2025

Refer #1061 for details.

Modified data/database.py as follows to match urllib API. Outgoing changes are commented with a # in the snippet below.

def download_from_url(url, destination_path=None):

--- SNIP ---

    # if response.status_code == 200:
    if response.status == 200:
        with destination_path.open("wb") as file:
            # file.write(response.content)
            file.write(response.read())
        return True
    else:
        return False

[x] I have read the CONTRIBUTING file.
[x] My PR is targeted at the dev branch (and not towards the master branch).
[x] I ran the CODE CHECKS on the files I added or modified and fixed the errors.
[ ] I have added the newly added features to News.rst (if applicable).

Copy link

welcome bot commented Jan 23, 2025

Thanks for opening this pull request! We'll make sure it's perfect before merging 🤗 force
Make sure to read the contributing guide. Also, if you think that your contribution is worthy of it, you can consider adding yourself to the Contributors list (feel free to ask us if you have any doubts).

@ignatious-p ignatious-p changed the title [fix] update data/database.py to use urllib methods. fixes #1061 [Fix] update data/database.py to use urllib methods. fixes #1061 Jan 23, 2025
@ignatious-p
Copy link
Author

ignatious-p commented Jan 24, 2025

[x] I ran the CODE CHECKS on the files I added or modified and fixed the errors.

I did this. It didn't show any issues then (only a recommendation to use a with statement as a context manager for the response variable). It gave a rating of 9.73/10 for the changes too.

I don't understand why the CI/CD checks fail; they seem to fail on unrelated files.

Let me try running the PyTest tests. I doubt it'd make a difference, but maybe I missed something in the 2 lines that I changed.

@DerAndereJohannes
Copy link
Contributor

If it runs fine on your system, it should be alright. Sometimes the CI/CD fails for some other reason which leads to some funny log files too. The failing pipelines should just be run again by a maintainer.

@ignatious-p
Copy link
Author

This is my first PR. If there is something else that I need to do on my end, please let me know. Bis dann!

@DominiqueMakowski DominiqueMakowski merged commit 957f681 into neuropsychology:dev Feb 11, 2025
4 of 8 checks passed
Copy link

welcome bot commented Feb 11, 2025

landing
Congrats on merging your first pull request! 🎉🍾 We're looking forward to your next one!

@ignatious-p ignatious-p deleted the dev branch February 11, 2025 11:58
@ignatious-p ignatious-p restored the dev branch February 11, 2025 11:58
@ignatious-p ignatious-p deleted the dev branch February 11, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants