Skip to content
This repository has been archived by the owner on Nov 10, 2024. It is now read-only.

More token clean up #529

Closed
wants to merge 4 commits into from
Closed

More token clean up #529

wants to merge 4 commits into from

Conversation

hadley
Copy link
Collaborator

@hadley hadley commented Mar 4, 2021

  • Deprecate set_renv in favour of cache
  • Default cache to FALSE
  • Export token_cache_path()
  • Import deprecated to match recommended style

Fixes #438

* Deprecate set_renv in favour of cache
* Default cache to FALSE
* Export token_cache_path()
* Import deprecated to match recommended style

Fixes #438
@hadley hadley requested a review from llrs March 4, 2021 13:44
@llrs
Copy link
Collaborator

llrs commented Mar 5, 2021

  • Deprecate set_renv in favour of cache: OK
  • Default cache to FALSE: Why? This would be another breaking change for users.
  • Export token_cache_path(): OK
  • Import deprecated to match recommended style: Which style do you refer to? There isn't any style set for this package.
  • Not sure I understand how these changes fix rtweet authentication not working for script running as a local job #438. Wouldn't the token generated with create_token would already be cached and used as default for subsequent calls (without this PR)?

@hadley
Copy link
Collaborator Author

hadley commented Mar 5, 2021

I think having create_token() overwrite a file on disk by default is extremely surprising; creating a token in one app, should not (by default) affect every other app. Yes, I think overhauling auth is going to break existing code, but in conjunction with #469, I think it's very much worth it.

Standard deprecation style is documented at https://lifecycle.r-lib.org/articles/communicate.html#deprecating-an-argument-providing-a-new-default-1

Hmmm, yes, maybe this doesn't fix 438, except to reduce confusion over exactly what is happening when you create a token.

For question that directly related to code, do you mind asking them comments in the PR? That makes it easier to follow the individual threads of discussion.

@hadley
Copy link
Collaborator Author

hadley commented Mar 6, 2021

Also might be better to just close this and #530, and attack in a single PR that addresses #469.

@hadley
Copy link
Collaborator Author

hadley commented Mar 8, 2021

Closing since I'm now pretty sure this isn't going to be useful once we've fully considered auth.

@hadley hadley closed this Mar 8, 2021
@llrs llrs mentioned this pull request Mar 22, 2021
@llrs llrs deleted the token-cache branch July 23, 2022 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rtweet authentication not working for script running as a local job
2 participants