psyinfra / onyo

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

`onyo new` accepts leading whitespace #123

Closed ghost closed 1 year ago

ghost commented 2 years ago

While testing how Onyo reacts to existing serial numbers, I typed " foo" as new serial, and the resulting filename had a space: headset_Corsair_Virtuoso RGB. foo

Nothing breaks because of it so far but I would avoid it - and I think I typed a space as I would have done when editing an asset file by hand.

TobiasKadelka commented 2 years ago

I think that was the state in which onyo was a few months ago, a decission was made to allow white space in all fields.

I am fine with automatically stripping the whitespace for each field, but that decission has then to be made actively.

Relevant in that context might be #70, since the python function gets the string pre-parsed from the terminal. Otherwise I would have said one could just do the same escaping for space with \ like normal, instead of making the choice which has to be made now.

ghost commented 2 years ago

I will need to think a bit about the consequences of each choice and will write in more detail (leading - middle - trailing whitespace)

TobiasKadelka commented 2 years ago

Cool, thanks a lot👌

aqw commented 2 years ago

I like the idea of allowing as much as possible (including whitespaces). That being said, I do see a lot of practical, day-to-day value in stripping any leading or trailing whitespace (not just spaces).

Edit: And if a user /really/ wants leading/trailing whitespace, they can escape it like other civilized folks. (e.g. \ foo\)

TobiasKadelka commented 2 years ago

I think they can't do that, because of #70.

There are a few projects on the internet that try to emulate the whitespace behaviour from the shell for python, and obviously also some that can handle escaping, but when I looked for a good solution, I did not find something that did the whole job. There were always some other input-strings that did not work then anymore in the ways we wanted it to work.

This is not to say, that I do not still want it, but that problem together with the idea of "allowing as much as possible" lead to the decission to have it not strip the whitespace.

aqw commented 2 years ago

I think they can't do that, because of https://github.com/psyinfra/onyo/issues/70.

Kinda. But they're completely different worlds.

70 is not a bug in my opinion.

This issue is solvable by simply running a trim on input.

TobiasKadelka commented 2 years ago

Yes, if we decide that we do not allow leading/trailing whitespace anymore. But I think we should allow it there, as much as we allow it in the middle of a field.

aqw commented 2 years ago

In my opinion, preserving leading or trailing spaces will confuse (and irritate) users. It is a common idiom to trim input, as users often copy-and-paste information from other sources.

Can you provide a concrete, practical use case where a user would prefer to have the strings un-trimmed? I cannot come up with one.

I propose that the default behavior trim, and allow literal spaces to be preserved via \.

TobiasKadelka commented 1 year ago

The new way for onyo new to behave is that it does not have the interactive dialog anymore, instead it expects a full directory and asset name. These are still allowed to contain leading whitespace and no trimming happens in the code, but these need to be escaped in the terminal, since the shell is now parsing the arguments. Therefore the incomming PR #243 will close this issue.