Skip to content

Provide user agent string when downloading data#25

Merged
xrotwang merged 4 commits intoconcepticon:masterfrom
johenglisch:user-agent-string
Apr 9, 2025
Merged

Provide user agent string when downloading data#25
xrotwang merged 4 commits intoconcepticon:masterfrom
johenglisch:user-agent-string

Conversation

@johenglisch
Copy link
Collaborator

Okay, @MiraAhmedovic reported a case where downloading a data set failed because the server required the request to provide a user agent string (Seems like it doesn't care what the user agent is as long as it gets one).

I just set the user agent to norare/1.1.0. Seemed resonable enough to me.

However, this PR is still incomplete. Fixing the code itself took me only a few minutes if at all but then I got stuck for over an hour trying to get the tests to work. I can't for the life of me figure out how to get pytest to run my mock implementation of the download function.

mock_impl = mocker.patch(
'pynorare.api.files.download_file',
sideeffect=make_pretend_data)
_main('download', 'dsid')
mock_impl.assert_called_once()

norare download seems to succeed and assert_called_once passes, so the function seems to run. But then norare map fails because the placeholder file isn't actually there… So, something seems to eat the side effects or something?

@xrotwang @LinguList Also, do you remember the reason why the old code tries download the data using the stdlib and then falls back to requests? Are there ever any cases where those yield different results?

@johenglisch johenglisch marked this pull request as draft April 3, 2025 11:29
@xrotwang
Copy link
Collaborator

xrotwang commented Apr 3, 2025

Re trying stdlib first: I can only imagine that I tried to get rid of the requests dependency - and failed.

@johenglisch
Copy link
Collaborator Author

Okay, I may be stupid. (<_<)" It should have been side_effect instead of sideeffect. The tests work now. (In my defence, the function should have thrown an invalid keyword argument error (<_<)" .)

Also, I replaced the download implementation with a stdlib one. So, pynorare has one dependency less now (no changes to setup.cfg though because requests was pulled in as a transitive dependency).

And I un-hardcoded the version number in the user agent string.

This PR should be good to go now.

@johenglisch johenglisch marked this pull request as ready for review April 4, 2025 08:36
@johenglisch
Copy link
Collaborator Author

Oh right, I keep forgetting that I don't have write access here – someone else has to do the actual merge (and probably release as well – at least I think that's what we did last time iirc).

@xrotwang
Copy link
Collaborator

xrotwang commented Apr 9, 2025

Just invited you.

@xrotwang xrotwang merged commit 818c847 into concepticon:master Apr 9, 2025
6 checks passed
@johenglisch johenglisch deleted the user-agent-string branch April 10, 2025 08:30
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.

2 participants