-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Create setup TUI and improve overall UX #125
Conversation
Also, I'd recommend putting an open-source license in this project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall great, just would recommend system cross-compatibility and .casefold() on some checks
✔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! I suggest that, in addition to checking for missing environment variables, there can be either an env variable or a dotfile that allows the program to know that the setup wizard has been run before.
For example, you can use the os.path.exists
function to check if .setup-run-before
exists. If not, it asks if the user wants to run the program. If it does, check for missing environment variables.
from os.path import exists
setup_done = exists(.setup-run-before)
so on and so fourth
I'll get this added, @owengaspard! |
Hey can you let me know when you add those! I would love to use this. |
Done! @Shadowles19 if you want to try this out, |
@Kamushy, @owengaspard, @JasonLovesDoggo I've added your changes! |
Ah, doesn't everyone love merge conflicts? I'll get this fixed @elebumm |
Alright, I'm going to add the new 2fa variable to the new setup script. Give me a moment. |
Unfortunately, the new color-coded console I added doesn't look like it will work with the new script. However, it does already have better logging than it used to. @elebumm this is ready to be reviewed! |
Merge conflicts are resolved. Now it's extra ready for review, @elebumm! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not appear there are any more conflicts.
I'm going to add the new variables to the setup assistant soon. |
Merge Conflicts fixed again. |
@blockarchitech Incredible PR. I had some issues with some of the merge conflicts. I also added the I will add this in straight away if we can quickly have those resolved. |
Thanks @elebumm! I don't see merge conflicts on my phone, can you clarify what you mean? Thanks! |
@blockarchitech I've solved them for you don't worry. They weren't major ones. |
Thanks @callumio! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just that one issue.
alright @callumio how's that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant, thank you for this, will really help simplify setup for users :D
Yeah, no problem! |
This pull request adds and/or changes the following:
Fixes/Closes #16