Password as CLI argument

[Himura2la]

I don't want to enter password every time. I created run-scripts with the host and file and I need it to run quick.

[dpgeorge]

It would be preferable to specify the password using a switch, like -p mypass (and it's not hard to do this without argparse). That's what users would expect. Additional positional arguments would tend to indicate that you are copying multiple files.

Also, please don't use f-strings, they will only work with very recent versions of CPython.

[Himura2la]

Multiple files are not supported so it's OK, one need a single glance on format to realize it. However, if you insist on -p, I can implement it.
I'll get rid of the interpolated string, OK.

[Himura2la]

I hope it's OK if -p argument must be the last one, trying to write another argparse will be rather complex task

[pfalcon]

I hope it's OK if -p argument must be the last one, trying to write another argparse will be rather complex task

Better to let it appear anywhere. And no, you don't need to write another argparse for this (e.g. it's ok to assume -p and the actual password will be separated by a space).

[Himura2la]

IMHO adding so much abstract arguments syntax is not a MicroPython way either, but OK, I'll try to do it with not very many lines and changes...

[pfalcon]

There's definitely another alternative, which we followed so far: to treat a webrepl client implementation here as a simple, reference one and let community develop "value added" tools using it as an example.

But if there's desire to add a password arg to the reference client, it apparently should be done to not potentially confuse users (not in an adhoc way).

[Himura2la]

"value added" tools are not good in this case, because passwords should never be hardcoded inside tools. Password should be near host address or nowhere.
Please check my last commit.

Following the no ad-hoc code way I also got rid of this e9ff506#diff-275e901131ecb58a8c9fee40909d64c0L224

There are still two unconditional print() calls whose aim I don't understand, maybe they are redundant on OUTPUT_LEVEL = 0

[Himura2la]

how about merge? *_*

[dpgeorge]

Thanks @Himura2la for updating the patch. But please note that the MicroPython philosophy of minimal and clean code applies not only to the core VM/runtime but also to the tools like this one. To that end there's a much simpler way to support optional args which allows to more easily add other options than the way you did it. Eg something like (the following is untested code):

passwd = None
i = 1
while i < len(sys.argv):
    if sys.argv[i] == '--':
        sys.argv.pop(i)
        break
    elif sys.argv[i] == '-p':
        sys.argv.pop(i)
        if i >= len(sys.argv):
            help(1)
        passwd = sys.argv.pop(i)
    else:
        i += 1
if passwd is None:
    import getpass
    passwd = getpass.getpass()

[dpgeorge]

No need to change this.

[dpgeorge]

No need to add this function.

[Himura2la]

I'm afraid

if 1:
    print("op:%s, host:%s, port:%d, passwd:%s." % (op, host, port, passwd))
    print(src_file, "->", dst_file)

is a kind of bad code. But if you insist to leave it, OK then. Or maybe we could add a parameter to debugmsg() ?

[dpgeorge]

It's customary to put options before the mandatory positional args.

[dpgeorge]

Please leave the original messages as they were. It particular you don't want to print out the password!

[Himura2la]

OK, what do you think?
I was not sure it's safe to modify sys.argv, but this really is much clearer code

[Himura2la]

@dpgeorge ping

[bhollosi]

patch working as intended, this is a huge improvement to use webrepl in a shell script.
merge pls!

[Himura2la]

@dpgeorge @pfalcon ping

[pfalcon]

@Himura2la : Instead of pop'ing values out of sys.args, it would be better to just parse it once and assign values to local vars. Because your approach doesn't scale if more options are added.

But what really precluded your patch to be merged is that it contained unrelated changes. If you want to add a password option, the patch should contain the minimal changes to achieve it, and not a character more.

Anyway, thanks for persistence, I cleaned up the patch and merged it.