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

politely should use on.exit to restore HTTPUserAgent #29

Open
DarwinAwardWinner opened this issue Jul 18, 2020 · 4 comments
Open

politely should use on.exit to restore HTTPUserAgent #29

DarwinAwardWinner opened this issue Jul 18, 2020 · 4 comments

Comments

@DarwinAwardWinner
Copy link

It looks like politely calls options to set the user agent prior to running the wrapped function and then calls options again afterward to restore the previous value. However, if the wrapped function throws an error, the second call to options never happens and the user agent is not restored. I believe the correct way to handle this is with on.exit, e.g.:

old_ua <- getOption("HTTPUserAgent")
on.exit(options("HTTPUserAgent"= old_ua))
options("HTTPUserAgent"= user_agent)
res <- mem_fun(...)
@dmi3kno
Copy link
Owner

dmi3kno commented Jul 18, 2020

Thanks for the tip. Will fix ASAP

@dmi3kno
Copy link
Owner

dmi3kno commented Aug 2, 2022

I am closing the issue, as the feature has been merged to the main repo. Feel free to open another one if you notice some other strange behavior.

@dmi3kno dmi3kno closed this as completed Aug 2, 2022
@DarwinAwardWinner
Copy link
Author

One minor note: it's generally good practice (as I've learned in the time since reporting this issue) to call on.exit with add = TRUE. This ensures that if you ever add a 2nd on.exit in the same function for unrelated reasons, both expressions will get run. Otherwise the 2nd one will override the first.

@dmi3kno dmi3kno reopened this Aug 2, 2022
dmi3kno added a commit that referenced this issue Aug 2, 2022
dmi3kno added a commit that referenced this issue Aug 2, 2022
@DarwinAwardWinner
Copy link
Author

Btw, I think this issue can be closed now. It looks like the PR merge didn't auto-close it.

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

No branches or pull requests

2 participants