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

feat: add logout function #595

Merged
merged 3 commits into from
Feb 6, 2023
Merged

feat: add logout function #595

merged 3 commits into from
Feb 6, 2023

Conversation

joshua-mo-143
Copy link
Contributor

  • add logout function for CLI

Reference: issue #594

Current queries:

  • api_key is currently defined as a String type, which means I've had to set the API key in the logout function as an empty string. This technically works, but I'm wondering if it's more correct to set it as None?

@oddgrd
Copy link
Contributor

oddgrd commented Feb 2, 2023

Nice! While it does work, I think your hunch is correct, it would be better to not just set it to an empty string. I'm thinking we could either change the set_api_key function to take an Option<ApiKey>, or we could add a new method, something like clear_api_key. I'm not quite sure what I would prefer 😄 Maybe you can try it out and see what feels best?

@joshua-mo-143
Copy link
Contributor Author

Nice! While it does work, I think your hunch is correct, it would be better to not just set it to an empty string. I'm thinking we could either change the set_api_key function to take an Option<ApiKey>, or we could add a new method, something like clear_api_key. I'm not quite sure what I would prefer 😄 Maybe you can try it out and see what feels best?

I decided to add a clear_api_key function in the end since I think that makes more sense for the purpose of modularity, in case we need to use it anywhere else.

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@oddgrd oddgrd merged commit 1a81711 into shuttle-hq:main Feb 6, 2023
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