--replace handling of commas in the old tag argument

[LeXofLeviafan]

It appears that the first argument to replace_tag() (and thus --replace as well) doesn't have a check for commas in it; thus leading to some peculiar behaviour:

• --replace foo,bar baz doesn't affect a record with tags bar,foo
• --replace bar,foo baz changes bag,bar,foo,qux to bag,baz,qux
• --replace bar,foo baz doesn't affect bar,barf,foo
• --replace ar,foo baz & --replace bar,fo baz doesn't affect bar,foo

I reckon that it should either reject such invocations altogether, or handle multiple tags in this argument properly.

[LeXofLeviafan]

Also, --replace foo,bar with no new parameter works on bar,foo but reports it as two records.

(Same goes for --replace foo,bar "")

[jarun]

We need to support 2 things:

• replace a tag with multiple tags (comma separated)
• remove a tag

I think we can add a new option to remove a tag explicitly instead of linking it to replace tag.

[LeXofLeviafan]

It might be a good idea, I guess (though I don't really care that much either way); this is regarding what happens if the first argument given to --replace contains commas.

I believe it should be either explicitly supported (and handled correctly), or explicitly disallowed (with attempts to do so being rejected).

[jarun]

We should explicitly ignore as it is ambiguous.

Also, you need to care about both the cli and api use cases as a collaborator and contributor. This is exactly the reason I am so defensive about maintaining status quo.

[jarun]

ignore - disallow.

[LeXofLeviafan]

This is exactly the reason I am so defensive about maintaining status quo.

If I didn't care about CLI and API, I wouldn't be asking to fix them when they're dysfunctional. What I said is that having or not having tag removal as a separate CLI option (as opposed to "replace with nothing") makes little difference to me, and it's not what this thread is about anyway. If you want to separate them, though, I'd suggest disallowing empty input for replacement (there wouldn't be much point to it otherwise).

ignore

"Ignoring" would mean "do nothing and pretend there was no action requested in the first place" :sweat_smile:. To "explicitly disallow" would involve telling a CLI user that he can't replace multiple tags with one, and an API call should signal a failure (which means either a failed result or an exception, depending on the general protocol… I guess in case of replace_tag() that would mean returning False on such input).

[jarun]

Yes, "explicitly disallow" is what I also prefer.

[LeXofLeviafan]

Incidentally, printing out the failure reason to stdout is not a good API behaviour (i.e. there's no sane way of capturing it to communicate in a network response or in GUI). The correct way would be to either return it or to pass it in a raised exception instead.

[jarun]

Yes, now we have to take that approach. When it was written, it was written with the CLI in mind.