Skip to content

Add IERS-specific remote timeout - #5111

Merged
taldcroft merged 1 commit into
astropy:masterfrom
taldcroft:iers-remote-timeout
Jun 17, 2016
Merged

taldcroft merged 1 commit into
astropy:masterfrom
taldcroft:iers-remote-timeout

Conversation

@taldcroft

Copy link
Copy Markdown
Member

Inspired by #5098 and #5099.

cc: @saimn @eteq @astrofrog

@taldcroft taldcroft added utils Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed labels Jun 17, 2016
the local ``iers.conf.remote_timeout`` value.
"""
with utils.data.conf.set_temp('remote_timeout', conf.remote_timeout):
return utils.data.download_file(*args, **kwargs)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

an alternative might be to do this instead of the with:

kwargs['timeout'] = conf.remote_timeout()
return utils.data.download_file(*args, **kwargs)

That should be the same, but it might be a bit less prone to trouble than set_temp? (Although maybe this isn't worth worrying about)

@eteq

eteq commented Jun 17, 2016

Copy link
Copy Markdown
Member

An alternative to my comment above: put the with ... set_temp... clause above all three of the existing download_file calls.

My main concern here is that someone else hacking on this might not understand why download_file is subtley different, but with the same name. (Yet a third solution, I suppose, is to change it to donwload_file_iers or similar.

None of that is a big deal, so if you like it as is, @taldcroft, I'm 👍 to merge this.

@taldcroft taldcroft added this to the v1.2.0 milestone Jun 17, 2016
@taldcroft

Copy link
Copy Markdown
Member Author

@eteq - I was on the fence about whether the name should be different, and even had def iers_download_file at one point. But as you say it doesn't make a huge difference here.

My version using the set_temp context manager has the slight advantage that in theory one could actually call it with a remote_timeout kwarg and that would get honored. In practice that probably won't happen, and the real the reason I did that was that was because I forgot about that option. In any case since you are good with merging, I think this is good enough.

@taldcroft
taldcroft merged commit 22a3339 into astropy:master Jun 17, 2016
@taldcroft taldcroft mentioned this pull request Jun 17, 2016
13 of 14 tasks
astrofrog pushed a commit that referenced this pull request Jun 18, 2016
@taldcroft
taldcroft deleted the iers-remote-timeout branch February 25, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed utils

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants