psyinfra / onyo

text-based inventory system on top of git
ISC License
3 stars 5 forks source link

Feature Request: allow `onyo mv <asset> <non-existing dir>` to create parent directories #655

Closed TobiasKadelka closed 3 weeks ago

TobiasKadelka commented 1 month ago

In the past, we decided that onyo mv mirrors the unix mv in so far that it errors when asked to move something to a non-existing destination. Now we begin using onyo in the real world for a real inventory, and I changed my mind about this command.

IMO onyo mv <asset> <non-existing-dir> should create all non-existing parent directories. When I create a new asset and directly asign it to a new user (which does not have a directory in onyo yet) with onyo new -d <non-existent>, then onyo prints the diff, which mentions the creation of both, the new directory and the new asset, and I can "Accept changes".

But if I have an existing asset, and I want to move it to a new user (which does not have a directory in onyo yet) with onyo mv <asset> <non-existent>, it errors and informs me that the move is illegal because the destination does not exist.

The arguments we had in the past for this that I remember were:

  1. "guarding against typos" -> But we print already a diff and ask the user explicitely to accept changes with onyo mv anyways, it guards IMO against nothing.
  2. "being closer to the unix mv" -> If I think about onyo commands as running python code that mirrors unix commands from the CLI, then this argument would make sense. But when I use onyo, I do not think about it that way; I think about it as inventorying. I have a device in front of me (or got the information from a employee), and I know to whom I want to asign it. In that moment I do not care about if the asset is "new" or simply "moved", and I do not think about if the user is "already tracked by the system" or if I am the first one to give them hardware.

In conclusion: this is why I think onyo mv should follow onyo new and simply create all new directories I specify as destination, and tell me it will do so, and then I as the user decide if that is what I want to do.

nhjjr commented 1 month ago

Good points, but it made me wonder why onyo new allows for the creation of non-existing dirs. The unix equivalents touch dir/file and cat > dir/file do not allow for that. Similarly, onyo mkdir creates (parent) dirs out-of-the-box, whereas the unix equivalent mkdir does not (unless invoked with the -p argument).

There are two important things to consider:

  1. Consistency. All functionality must be consistent. If we want to mimic functionality of unix commands (and document it like that), then we should do so for all onyo functions.
  2. Usability. Is the consistency we keep with unix commands more beneficial than the 'extra' functionality that is now suggested?

As for point 2, I think we can argue in both ways. I think there is use in giving all of these functions by default the ability to create (parent) dirs. However, we cannot "half-ass" this. If we give one this ability, then we should give it to the other.

On the other hand, if we deem consistency with unix functionality to be more important/beneficial, we should remove the creation of (parent) dirs from onyo new and add a -p to onyo mkdir when it is to be used to create parent dirs.

There is merrit to sticking to unix functionality. We assign meaning to directories--in our case this is location of the asset--so keeping the creation of a directory as a separate record has value. Otherwise, the creation of a location or user will sometimes be an onyo mkdir record, sometimes an onyo new record, and sometimes an onyo mv record. One could of course do onyo mkdir ... && onyo mv ... to still solve things in one line.

Does this merrit outweigh the usability Tobi suggests? I do not know.

aqw commented 1 month ago

Ironically, I mostly have arguments in support of this change, though my opinion is largely against this. Perhaps that is simply because I have not used Onyo in workflows that would expose me to the usefulness of this change. More example use cases would help. I do find the idea to be unintuitive, and removes what I find to be useful feedback (error when the target parent dir does not exist).

The unintuitive concern comes down to that it will behave differently than the traditional unix mv command. To address that, if this feature is accepted, I propose renaming onyo mv to onyo move.

My primary concern is typos/errors. I imagine the file system as a scaffolding. We build it intentionally, and when we need another space/person, we explicitly add it.

Diffs are not a magic catch all. Users largely will skim the output, and I suspect that most of us, when making a typo would not catch in the diff that the target dir is new. This can be improved upon with #656 (and possibly also #657) to help highlight significant changes.

As for consistency across commands, we can simply remove directory creation from onyo new to make it consistent. But I don't find it all that inconsistent to have new create. What I chew on is the idea that onyo set be allowed to set the directory pseudo-key. If we end up adding that, I do believe that it should create any needed parent directories. But somehow, that just /feels/ different than onyo mv.

I think it's because set isn't operating on the filesystem. It's operating on data/metadata. But filesystem operations, I imagine to work like the filesystem traditionally does.

Those are my thoughts. I'm not sure which direction they are headed. I'll continue to meditate on this. Additional input most welcome.

TobiasKadelka commented 1 month ago

Diffs are not a magic catch all. Users largely will skim the output

Here I might use onyo different than others, but I do spend a lot of time reading the diff and checking that everything is correct. And I would expect Jan and Ralf do the same (even though I have no proof for this part).

we can simply remove directory creation from onyo new to make it consistent

You are not the first person to state something like this. I am strongly against this change. The idea to make onyo even just a little less comfortable to use just for the sake of an idea of consistency makes me shiver. I do however like the idea to allow onyo set to be more power for the same reasons as listed above for onyo mv ;)

I propose renaming onyo mv to onyo move

If that is something that would help others, I am definitely happy with this change, even without the feature requested in this issue :)

More example use cases would help

Frankly, I do not have that many more example use cases, it always boils down to the same scenario:

I do not even have a "strong need" for this change to happen, for me it just would be a little convenience. But currently this is a little rough edge in onyo for me; not a big problem, just a small annoyance each time, but regularly. We now have most rooms tracked, but new users will join all the time and then we will give them their first piece of equipment, and this inconvenience happens every time. I know Jan made the same experience. For me it boils literally down to "the two people who moved the most hardware in the real world by now made the same experience and did not find it positively great".

-> If everybody else is against it, we will learn to live with it, but I personally like the direction you bring up of asking if there are other changes that would make this this more intuitive. Renaming the command to onyo move, adding bold highlighting and the operations records to the diff, and possibly allowing onyo set additional abilities are probably all good ideas on their own and might help this feature to feel more natural, too.

TobiasKadelka commented 3 weeks ago

After having experienced how the "Inventory Operations" now get printed too, I want to come back to this issue and re-emphasize how good of an idea this issue still seems to me. ;)

It is a simple addition that makes onyo just a tiny bit smoother to use, it introduces no problematic complexity, and it fits very neatly together with #693 :)

bpoldrack commented 3 weeks ago

I'll refer to my comment on this line of thought elsewhere.

I think this approach breaks fundamental paradigms and I really, really do not like it. At all.

Calling this "consistency", b/c another command can do this, is complete nonsense to me. One might as well say "rm can remove an asset", so why can't mv? We must have onyo mv asset /dev/null!!! What has new to do with mv? One command is about creating things that don't exist yet and the other is about moving things. Yes, onyo mkdir has -p as default (in opposition to a plain mkdir). So, what? Still it's a command whose very (and only!) purpose is to create dirs. But b/c it does this, a completely different command also needs to?

So - no way to fail on typos or other confusion that led to an invalid destination. The only way to be safe is to not make any mistakes. Which is fine, b/c "we ask". So, yeah, only ever using mv interactively and always very carefully reading the diff, because there might be something in it that's not actually a move is the solution and the desire here?

I know I'm ranting, but I really hate this.

bpoldrack commented 3 weeks ago

May be one more remark on being able to properly fail if the destination doesn't exist:

We ourselves use users as "locations". And there are people from all over the world. Now, you want to move something to them but you confuse first and last name. Are you going to notice that in the diff? Most likely no, for the same reason you confused it in the first place. But that one move may not be the only interaction with that user. As a consequence we get two users with first and last name swapped. Who is going to notice when?

What the inability to fail on non existing destinations implies is: You want to record every freaking typo you ever made in such a call in the inventory's history. And rely on someone figuring out that this wasn't intentional and what actually was the intention at some unknown point in the future. This completely contradicts the aim of properly safe-guarded inventory. A location is a tracked item. It better be created in a conscious decision, not as an implicit result of moving things.

aqw commented 3 weeks ago

[...] record every freaking typo [...] in the inventory's history. [...] This completely contradicts the aim of properly safe-guarded inventory. A location is a tracked item. It better be created in a conscious decision, not as an implicit result of moving things.

I agree strongly with this point.

I have explored a related space a bit in https://github.com/psyinfra/onyo/issues/693#issuecomment-2456679594

In short: I think this feature should not exist in onyo mv. If onyo set can indeed do this, then the value of mv/mkdir/rm is that they have these safeguards and error. If we /don't/ go that route, then I still think mv should not do this, because it has nothing to do with creation (which is the onyo new exception).

TobiasKadelka commented 3 weeks ago

Well, the thing is that I still think this would be a good feature, and I do not think the likelyhood for errors through typos is here a bigger risk than anywhere else, I think because of diff output and inventory operations output it is more likely to be noticed than most other typos.

That said -- Clearly you both agree on this beeing a bad idea. I personally think it would be only a minor convenience; I would have liked to have it, but I can clearly live without it.

Thereby I will use my authority as creator of this issue and simply close it so that we can focus on more important ideas. Thanks for considering it!

aqw commented 3 weeks ago

Haha. Sounds good. Thanks for the idea and feedback!