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

Setter of Access token #45

Closed
wants to merge 2 commits into from
Closed

Conversation

anshulg954
Copy link
Collaborator

@anshulg954 anshulg954 commented Oct 28, 2024

Change Description

Try to be precise. You can additionally add comments to your PR, this might help the reviewer a lot.
Enhancement to set the access token if the user already has it.

  • The user can now set the token via init(access_token="access_token"). If the access token is valid we will use it, otherwise the pre-existing prompting will continue.
  • Also the user can view his/her token by get_token() directly.
  • Additional Information regarding access token can also be found in all the above functionalities.
  • Test cases have been updated and added.
  • Readme.md has been updated as per functionality.
  • Fixed Issues in resetting token. Earlier it was just deleting the file but not clearing the existing cache and was not updating the existing config.
  • Also gave a temporary fix for reverify email() which has been fixed thoroughly in hashing PR.

If you used new dependencies: Did you add them to requirements.txt?

Who did you ping on Mattermost to review your PR? Please ping that person again whenever you are ready for another review.
@liam-sbhoo

Breaking changes

If you made any breaking changes, please update the version number.
Breaking changes are totally fine, we just need to make sure to keep the users informed and the server in sync.

Does this PR break the API? If so, what is the corresponding server commit?
No

Does this PR break the user interface? If so, why?
No

Please do not mark comments/conversations as resolved unless you are the assigned reviewer. This helps maintain clarity during the review process.

@anshulg954 anshulg954 requested a review from liam-sbhoo October 28, 2024 12:19
@anshulg954 anshulg954 force-pushed the setter_getter_access_token branch from 3a593b1 to 4342114 Compare October 30, 2024 21:44
Copy link
Collaborator

@liam-sbhoo liam-sbhoo left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

I think we could add one more feature, where the user can set the token directly without having to go through the prompting. One example is, tabpfn.init(access_token='blablabla').

@noahho
Copy link
Collaborator

noahho commented Nov 7, 2024

This is a misunderstaning in how the access token is sued. We only want to set the access token using a functional interface, so by calling tabpfn.set_token(...) not using the prompt interface. The token is useful if you want to programatically login, so I think we don't need any new code, but only add to the README doc how to login using the token

@anshulg954 anshulg954 force-pushed the setter_getter_access_token branch 2 times, most recently from c8f717c to 68a0cb5 Compare November 12, 2024 10:44
Copy link
Collaborator

@liam-sbhoo liam-sbhoo left a comment

Choose a reason for hiding this comment

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

Can you add some test cases? Also perhaps a demonstration on this work.

P.S: When you create a PR, would be nice to provide the reviewer an idea/demonstration of the new workflow.

@anshulg954 anshulg954 force-pushed the setter_getter_access_token branch from 557a0f5 to 3613acf Compare November 13, 2024 20:29
@anshulg954
Copy link
Collaborator Author

anshulg954 commented Nov 13, 2024

Hi Liam!
I did not think it as of a major change the first time I created this PR as it was a few lines of change. However, later forgot to modify the description about the changes. Sincere apologies for the same.

As per your comments, I have updated the code and added test cases.

@noahho noahho dismissed liam-sbhoo’s stale review December 11, 2024 09:14

no reply, changes done

@SamuelGabriel
Copy link
Collaborator

I think this PR does not have much new stuff compared to the current main. I would close this, ok? Sorry about this

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.

4 participants