Flags #17
Flags #17
Conversation
|
Can you remove the 'Revert doc change ...' commit from this? |
| maxsize = max(len(str(f.size)) for f in files) | ||
| for f in files: | ||
| mode = ( | ||
| ('d' if stat.S_ISDIR(f.mode) else '-') + |
There was a problem hiding this comment.
There's a stat.filemode function in Python 3.3+ :(
maruel
force-pushed
the
flags
branch
2 times, most recently
from
f85d3ed
to
397b762
Compare
|
I made it magical. I hope I get bonus points! :) Please test before merging. I did test it (and fixed adb_test.py) but I'd prefer to have a second check before getting this committed. |
|
Ping, as I have another PR immediately following but I want to base the other PR on this one. |
|
|
||
|
|
||
| def Logcat(self, *options): | ||
| return adb_commands.AdbCommands.StreamingShell( |
There was a problem hiding this comment.
Is this quoting absolutely necessary? If they put such characters into the shell interface, they're ruining their own system. If someone is taking raw inputs and passing it to adb or python-adb, they're in trouble with or without this
There was a problem hiding this comment.
If you want it to stay, replace the doc line with functools.wraps
|
Sorry, I'm OOO at the moment so I can't do a more thorough review. I'm most interested in the arg parsing and if it's too complicated or not, so I'll be looking at that on Wednesday when I get back in |
|
Ok thanks, I'll wait for your review. |
|
|
||
| @functools.wraps(adb_commands.AdbCommands.Logcat) | ||
| def Logcat(self, *options): | ||
| return adb_commands.AdbCommands.StreamingShell( |
There was a problem hiding this comment.
This and Shell don't do anything. Shell just renames StreamingShell, but that can be done in MakeSubparser
There was a problem hiding this comment.
They are needed.
Shell() exists to adjust the docstring as it has to be different, there's no 'yield' concept at the CLI.
Logcat() -> it's really just to merge the outputs.
I looked at modifying MakeSubparser() to be smarter about merging remaining arguments but it's not trivial, at least not in a way that makes the code more opaque than doing the two functions.
The point here is that by doing too much magic, we're making the code less readable and less maintainable. Having these few adaptor functions actually helps readability compared to adding more magic inside common_cli.py, IMHO.
There was a problem hiding this comment.
If shell just changes the docstring, then it should just call the other function directly so it's simpler. Logcat should also call to the existing Logcat, but with the joined options. I meant to only renamed StreamingShell to Shell in MakeSubparser, but if it's doing other things then never mind.
There was a problem hiding this comment.
LogCat() is still needed because the *options makes MakeSubparser use argparse.REMAINDER. That's why I don't want to make the code too magical, it makes it much header to reason about it.
I did replace the StreamingShell() to Logcat() call. I also fixed it in adb_commands.py.
There was a problem hiding this comment.
Yes, I meant to replace the inside of the function with a simpler call, not to remove it.
|
Almost there, sorry about all the changes. Thanks for the docstring stuff, though. It's hacky, but a lot less copied code. Maybe in the future we can remove docstring-parsing for something else? |
|
Pushed new patchset |
|
One last comment, sorry about the delay! |
It is more appropriate for open source projects to use argparse. Keep the 'clever' approach and uses the docstring on methods on AdbCommands/FastbootCommands to generate the arguments on the parser. Override the documentation only when it doesn't make sense, like when an argument can be "object-like". Update default fastboot packet size from 4kb to 1Mb. Fix tests (adb_test.py was broken). Make them executable.
|
Sorry about the back-and-forth! Thank you! |
Switch use of google-flags (gflags) to more standard argparse. Keep the 'clever' approach and use the docstring on methods on AdbCommands/FastbootCommands to generate the arguments on the parser. Override the documentation only when it doesn't make sense, like when an argument can be "object-like". Update default fastboot packet size from 4kb to 1Mb. Fix tests (adb_test.py was broken). Make them executable.
If you want, you can merge this one instead of #16, as this merge request includes both.
Please try it locally first. I tried almost all commands on adb_debug.py and a few commands on fastboot_debug.py; getvar all and continue, as I didn't want to flash the device I was playing it. At least I confirmed I can access the device via --port_path in fastboot_debug.py.
This PR will remove the conflict in luci/python-adb about stripping gflags; this will ease the upstreaming process.
I made "./adb_debug.py list /path" output in a nice way as a bonus. I removed 'stat' since it was not very user-friendly.
The subparsers are still a bit too clever to my taste but if I hadn't done that, the code length would have exploded. At least the proposed design makes the help pages much more pleasant to read.