micropython / webrepl

WebREPL client and related tools for MicroPython
MIT License
633 stars 297 forks source link

Password as CLI argument #34

Closed Himura2la closed 7 years ago

Himura2la commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

pfalcon commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

"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 https://github.com/micropython/webrepl/pull/34/commits/e9ff50607f0b6924b5ba80f31ee274f276b903fb#diff-275e901131ecb58a8c9fee40909d64c0L224

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

Himura2la commented 7 years ago

how about merge? *_*

Himura2la commented 7 years ago

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

Himura2la commented 7 years ago

@dpgeorge ping

bhollosi commented 7 years ago

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

Himura2la commented 7 years ago

@dpgeorge @pfalcon ping

pfalcon commented 7 years ago

@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.