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

Added keyring usage #22

Merged
merged 12 commits into from
Sep 16, 2024
Merged

Added keyring usage #22

merged 12 commits into from
Sep 16, 2024

Conversation

sepehrrasooli
Copy link
Contributor

This PR will close Issue #19, I changed the token storage system from environment variables to keyring, Added a new hey token [HEY_TOKEN] command and changed README.MD to reflect the new changes.

@sepehrrasooli
Copy link
Contributor Author

sepehrrasooli commented Sep 13, 2024

Hey @lnxpy, This PR has one downside, if the user has used the hey token to store their token in the past, hey will retrieve the previously stored token, This might cause some inconvenience.
Do you think we should add a FIRST_RUN variable to the config file to notify the user of this problem ?
Also, should we add an hey token --remove-token command ?

@lnxpy
Copy link
Owner

lnxpy commented Sep 13, 2024

Also, consider updating the README file. Thanks, man!

@lnxpy
Copy link
Owner

lnxpy commented Sep 13, 2024

Hey @lnxpy, This PR has one downside, if the user has used the hey token to store their token in the past, hey will retrieve the previously stored token, This might cause some inconvenience. Do you think we should add a FIRST_RUN variable to the config file to notify the user of this problem ? Also, should we add an hey token --remove-token command ?

Isn't it better if we set the set_token function first to remove all the keys named hey and then set a new one? It'll fix the issue I guess.

@sepehrrasooli
Copy link
Contributor Author

sepehrrasooli commented Sep 13, 2024

Hey @lnxpy, This PR has one downside, if the user has used the hey token to store their token in the past, hey will retrieve the previously stored token, This might cause some inconvenience. Do you think we should add a FIRST_RUN variable to the config file to notify the user of this problem ? Also, should we add an hey token --remove-token command ?

Isn't it better if we set the set_token function first to remove all the keys named hey and then set a new one? It'll fix the issue I guess.

No, This is exactly what the program does currently with no added lines of code,

I believe the issue I was trying to solve was too much of a hassle to implement, and a very impossible scenario to happen.
Let's forget it altogether.

Also, consider updating the README file. Thanks, man!

I already did, You're welcome :).

@lnxpy
Copy link
Owner

lnxpy commented Sep 13, 2024

We'll look into that issue later. Pay attention to the reviews that I left. Once they're satisfied, then consider updating the README. @sepehrrasooli

hey/cli.py Outdated Show resolved Hide resolved
hey/openai.py Outdated Show resolved Hide resolved
@sepehrrasooli
Copy link
Contributor Author

sepehrrasooli commented Sep 15, 2024

Hey @lnxpy !, I just made the changes you requested, hey auth will now not echo the token and the error message in TokenIsNotDefined has been changed, I also updated the README.MD file.
Please review my code and request changes if necessary :).
P.S.: I changed hey token to hey auth as you requested.

…th, also made minor changes in other files to reflect it.
@lnxpy lnxpy linked an issue Sep 16, 2024 that may be closed by this pull request
lnxpy

This comment was marked as spam.

hey/openai.py Outdated Show resolved Hide resolved
hey/cli.py Outdated Show resolved Hide resolved
hey/cli.py Outdated Show resolved Hide resolved
Copy link
Owner

@lnxpy lnxpy left a comment

Choose a reason for hiding this comment

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

They are awesome. Some minor changes are required though.

@sepehrrasooli
Copy link
Contributor Author

They are awesome. Some minor changes are required though.

Thanks, I made the requested changes :).
Please review them accordingly.

Copy link
Owner

@lnxpy lnxpy left a comment

Choose a reason for hiding this comment

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

All perfect. :))
Awesome work man!

@lnxpy lnxpy merged commit 9dec823 into lnxpy:main Sep 16, 2024
4 of 5 checks passed
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.

Change token storing system from bashrc to keyring python library
2 participants