PyBites-Open-Source / eatlocal

This package helps users solve PyBites code challenges on their local machine.
MIT License
20 stars 9 forks source link

Init mode #27

Closed pmayd closed 3 weeks ago

pmayd commented 2 years ago

I just thought about an init mode. This could be used to ask the user for username and password so they don't have to create env vars before using eatlocal. In addition we could register the current working director as bite repo, or ask the user about this, too. So all three constants would be stored once you run eatlocal init. And if you want to change any of the three you can run the command again. Not sure if we can permanently set env vars with os.env, I think this was only temporarily, but I propose to create a .eatlocal/.credentials file in the home directory that is read at the beginning of each run of eatlocal to have the latest variables. Maybe there is a better way so I have to research how to set env vars with python but this would make the tool easier for people who are not familiar with this kind of things

rhelmstedter commented 2 years ago

I love this idea. I'll create an init branch and test out your commit.

rhelmstedter commented 2 years ago

One issue that I noticed is that it did not expand the ~. I tried setting it up on my work computer running linux. I will see if typer has something built in to handle this.

image

pmayd commented 2 years ago

OK I am a little lost here, what is happening and where does ne "~" come from? It seems Path().home() is correctly resolved to /home/deanza/.eatlocal, as the green text implies. But your current working directory is ~/eatlocal? So the download Path is not correct but this has nothing to do with the init command right? The init command creates a .env file in the .eatlocal hidden folder and that has worked because the download command successfully downloads the bite so the user credentails where correctly found and read in.

Ah maybe the repo link is wrong? Your screen does not show the very important line BEFORE the confirmation ask, what is the content of the repo variable? Can you show me the value of PYBITES_REPO? I implemented it so that if you don't give a specific path it takes the current working directory and maybe because the current working directory is ~/eatlocal he stored it with the "~". So the fix is easy, I have to use resolve() as this is the only reason you need this method when working with Path, to resolve the user directory "~".

pmayd commented 2 years ago

So we have to use .resolve() when storing the current working directory for the repo variable as default in the init command.

rhelmstedter commented 2 years ago

Your screen does not show the very important line BEFORE the confirmation ask, what is the content of the repo variable? Can you show me the value of PYBITES_REPO?

Sorry, I had to crop that piece because it displayed my actual password. I had typed in ~/pybites.

~ is a unix short hand for the user home. When I tried it with .resolve() it was creating a new directory in my current working directory /home/user/eatlocal/~/pybites. I got this to work using os.path.expanduser().

image

pmayd commented 2 years ago

Yep that is what I described, if you enter such a path there might be a problem but this has to be possible with Path to, os is considered deprecated and should not be used unless necessary and the documentation of home() references os expanduser so I will test it

rhelmstedter commented 2 years ago

Ooo I just saw there is a Path.expanduser()

pmayd commented 2 years ago

Yes but this should be identical to home()

pmayd commented 2 years ago

OK you are right but I don't get it why resolve() is not doint this... If you have a path that starts with "~" you need:

>>> p = PosixPath('~/films/Monty Python')
>>> p.expanduser()
PosixPath('/home/eric/films/Monty Python')

So I guess we have to check if the path starts with that

pmayd commented 2 years ago

It seems that expanduser is not doing anything to a Path without "~" this is great! And even under windows it works! The normal cmd does not support "~" like UNIX systems but Path allows it!

Path("~/.eatlocal").expanduser()
Out[13]: WindowsPath('C:/Users/micha/.eatlocal')

This means we just have to use expanduser() when we access config["PYBITES_REPO"] or even better before we store it in the init command

pmayd commented 2 years ago

Should be fixed by the new PR

rhelmstedter commented 2 years ago

Awesome. Just merged it and am super excited. I think it will be was easier for new users. Thanks for doing this.

Some thoughts: Are there security concerns with displaying the password? Would it be better to use the confirm option in typer?

pmayd commented 2 years ago

Good question but who is using this on a public computer ? And it's only when you initialize the first time or use this command, afterwards your password is no longer displayed. But yes, you have to type it in at some point but the same is true for the env vars you had to create at some point. I don't think we have to be alerted here because it's only local and never transmitted in plain text (I don't know how selenium works and what the driver does I am just talking about the .env file now). But I already used the Prompt.ask() with password=True Parameter that is hiding the input. But I know that I am destroying this right afterwards when displaying the input for confirmation haha. I knew you would found this odd, but I wanted to make sure there is no problem with the input. This is important because the download command does not tell of the credentials are correct, the download just won't work but you are not told the reason why. We could omit the display if the user input as you suggest and ask the user to make sure that the content of the env file is correct but I don't think that our package will be used in any real world scenario where anyone could steel your pybites credentials...

pmayd commented 2 years ago

But sure, feel free to look for alternative approaches ;)

rhelmstedter commented 2 years ago

This is important because the download command does not tell of the credentials are correct, the download just won't work but you are not told the reason why.

This makes me think I should look into checking that the credentials are accepted on codechalleng.es.

We could omit the display if the user input as you suggest and ask the user to make sure that the content of the env file is correct but I don't think that our package will be used in any real world scenario where anyone could steel your pybites credentials...

I am fine with displaying everything as long as people using the tool don't have concerns. It just feels odd to hide the password during input, only to display it later. I think we should either keep it hidden, while asking for confirmation to help with typos, and not display it. Or just display it the entire time like the user name.

pmayd commented 2 years ago

Yes, my thoughts exactly.

Maybe we do a two-fold confirmation so you have to enter your password two times, hidden, and only if they match and you confirm the other two variables, we move on

Russell @.***> schrieb am Fr., 29. Apr. 2022, 22:41:

This is important because the download command does not tell of the credentials are correct, the download just won't work but you are not told the reason why.

This makes me think I should look into checking that the credentials are accepted on codechalleng.es.

We could omit the display if the user input as you suggest and ask the user to make sure that the content of the env file is correct but I don't think that our package will be used in any real world scenario where anyone could steel your pybites credentials...

I am fine with displaying everything as long as people using the tool don't have concerns. It just feels odd to hide the password during input, only to display it later. I think we should either keep it hidden, while asking for confirmation to help with typos, and not display it. Or just display it the entire time like the user name.

— Reply to this email directly, view it on GitHub https://github.com/rhelmstedter/eatlocal/issues/27#issuecomment-1113710171, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACJLHU72N6BTPZDKB7HEOEDVHRCRHANCNFSM5UUVRA5A . You are receiving this because you authored the thread.Message ID: @.***>

rhelmstedter commented 2 years ago

Yes, I agree. Do you want to handle that or should I?

pmayd commented 2 years ago

Feel free :)

rhelmstedter commented 2 years ago

This might be a dumb question, but where did you find the documentation for Prompt.ask? I couldn't find it by searching on https://typer.tiangolo.com/.

Edit: And it was... Prompt is from rich not typer. I guess I should have looked at the imports first :)

pmayd commented 2 years ago

Yes the documentation is lacking a lot in this regard I went strait to the source code where all attributes a described properly

rhelmstedter commented 2 years ago

Found a nice little bug too. Back on my home machine where I have no .eatlocal/.env file, I get the warning to run eatlocal init even though I am. I believe this is happening because __main__.py is trying to import config from constants.py.

image

pmayd commented 2 years ago

This is not a Bug but the expected behavior of how I implemented it. eatlocal is checking before anything else if it can load the .env file and if that is not possible ,displays the warning. I am not sure how to check if the current command is init and not display this warning. We would have to delay the check of the .env file to later, but we need it in constants so that the necessary variables can be loaded...

pmayd commented 2 years ago

Exactly, constants.py is loaded at the beginning even before eatlocal is doing anything, so I used this to inform the user that eatlocal is not yet fully functional. The good thing is that this warning will disappear once you have run the init command successfully :P

rhelmstedter commented 2 years ago

hmmm can we import load_config() and just call it in all of the commands except init? And if the env_path doesn't exist display the warning and exit. Currently it will display the warning but continue trying to download (or whatever). Or am I overthinking it? 😂

image

pmayd commented 2 years ago

Absolutely, I guess I wanted it to be done before any command so that it is happening on a global level and I am sure there is some way to do this, decorator for example would be a nice exercise but yes this would be a better approach and we avoid that the commands go forth.

Originally I had implemented it directly in the first check so that the program would exit...realizing only later that now it was impossible to ever call even the init command haha.

I can do it or leave it to you

rhelmstedter commented 2 years ago

realizing only later that now it was impossible to ever call even the init command haha.

Same! I almost made it so that was the case, haha. I'll attempt it :). It's good for my learning.

rhelmstedter commented 2 years ago

here it is: 35cc0270caee94b24055547a2468a3f5479ca1ef

pmayd commented 2 years ago

commented, but otherwise fine!

Zaubeerer commented 2 years ago

eatlocal init seems to not have worked properly for me, as there is no file created in /../.eatlocal 🤔

ghost commented 2 years ago

eatlocal init seems to not have worked properly for me, as there is no file created in /../.eatlocal 🤔

The config file should be created in your user folder, not in some parente folder, except that is identical for you in this particular case. Otherwise, pls check if the config was created under ~/.eatlocal/.env

ghost commented 2 years ago

Did you get a success message? print(f"[green]Successfully stored configuration variables under {EATLOCAL_HOME}.") What was the content of the variable EATLOCAL_HOME for you?

Zaubeerer commented 2 years ago

The config was successfully created and contains the required information :)

On my Mac M1, eatlocal now works perfectly.

However, on my Windows machine, there are still issues.

Not sure, whether this is related to the initialization or something else, so I created this issue https://github.com/PyBites-Open-Source/eatlocal/issues/31.

rhelmstedter commented 2 years ago

@Zaubeerer Just responded to your other issue. I'll leave this open until we completely rule out the init mode as the issue on your Windows machine.

Zaubeerer commented 2 years ago

Just solved the problem on the windows machine.

See #31 for more information.