archlinux / archinstall

Arch Linux installer - guided, templates etc.
GNU General Public License v3.0
6.33k stars 543 forks source link

Changing sys_command to take named keyword arguments. #68

Closed vamega closed 3 years ago

vamega commented 4 years ago

Just getting back into this project, have been busy wrapping up some things for work.

One of the things I noticed the last time I was diving into this project, is that the sys_command class has a huge implict set of dependencies on things passed in via kwargs. Is there a reason for this?

It seems like it only ever looks at a few keys within kwargs

Having those be explict arguments should make things easier to document. I also worry a bit about kwargs being passed through. For example right now the all_disks function passes through kwargs, but it's not clear that the kwargs for all_disks should be passed to sys_command, and most likely the extra partitions key should not be passed along.

if you're amenable to this I can fix this and send over a PR.

vamega commented 4 years ago

I read through the implementation of sys_command but I'm unable to grasp what the emulate option is supposed to do. Would you mind explaining what it's purpose is?

Torxed commented 4 years ago

Correct, there is a history behind this. sys_command() was considered a very transparent execution class/function for system commands. I designed it so that it would be able to take anything, but only execute a subset of parameters.

For instance,

archinstall.luks(partition="/dev/sda2", mountpoint="/mnt", password="xyz", emulate=True)

Would in theory make the sys_command() emulate the command. Which perhaps answers the second question, what emulate does. I used to throughout the development phase to emulate the execution chain, like a unit tests. And to some extent did up until very recently. It was a pretty quick and efficient way to make sure command calls (even on abstract archinstall level) never actually got executed (potentially ruining my harddrive if I tested things "on the go").

So that's the back story. Now to the future plans. You've manage to find the functionality I'm probably least proud of hehe. And it should be re-worked a bit. Especially the __iter__ function, I thought it was a nice thing because I could use it with string manpulation and stuff. But I've realized that the best way forward would probably be just a standard .output() or something which builds/joins the results together. It was also there for this purpose:

for line in sys_command("ls -l"):
    print(line)

Which was nice for debug outputs and stuff. But I don't think we need that anymore. I would need to consider the nature of sys_command's diffuse parameter-requirement a bit, but as long as the function calls before sys_command takes *args and **kwargs I don't see a problem making sys_command taking more strict parameters. Since I would like early function calls to propegate down through the function calls to the end-call, and return back results all the way up.

tl;dr: There's nothing saying (any longer) that sys_call has to utilize the **kwargs method of getting those, so go for it. If you want to clean that beast up, I'd welcome the change : )