PICARD-2526: Allow starting processing actions from the command line

[skelly37]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Users will be able to pass some commands to the existing instance with -e/--exec flag.

Problem

It would be useful to tell the existing Picard instance what to do, e.g. QUIT, useful in combination with the other single-instance mode improvements

• JIRA ticket (optional): PICARD-2526

Solution

• add command:// prefix to the commands
• send them as any other arg
• parse with urlparse, split by ";"
• execute

Action

[skelly37]

Hm

• The core works
• SHOW works
• QUIT gets stuck as when the pipe was introduced, then removed (but force quit will not be a reasonable solution here)
• print leftovers removed
• the cli flag got refactored

Time to implement other commands and think about the QUIT in the meantime, I guess...

(Sorry for the bad responsivity btw, I'll finish moving on Saturday/Sunday)

[zas]

@skelly37 skelly37#5 adds a bit more logging

[skelly37]

Update:

• QUIT got fixed and has a proper flow
• 5/9 commands are already implemented (QUIT + SHOW + all the cluster pane ones)
• Only album pane ones left to do
• After that we can discuss ideas/design of the functions that actually take the arguments suggested by @zas

@phw, @zas What's wrong there? I don't understand...

[zas]

@phw, @zas What's wrong there? I don't understand...

This was a temporary server issue (it was returning http 500) when trying to upload SARIF results from CodeQL action.
This is configured by this workflow.

I re-ran the failed job, and it worked.

[zas]

@skelly37 your last commit is titled "consider it ready", it wouldn't help one looking at commit history.

In general, I think you should be more careful about your commit messages.Things like "almost done" or "typo fix v2" don't help anyone to understand what's the commit is about.
Also, please use correct English (capitalization, punctuation, verbs).

In this case, since that was a draft pull request for a while, I think an interactive rebase / history cleanup is needed before merging.

This was also the case with your others PRs. Nothing catastrophic, but we usually prefer clean explicit commit messages (and sometimes we get lazy too, so our commit messages aren't always fantastic...).

Please just take care about that.

[skelly37]

@skelly37 your last commit is titled "consider it ready", it wouldn't help one looking at commit history.

In general, I think you should be more careful about your commit messages.Things like "almost done" or "typo fix v2" don't help anyone to understand what's the commit is about. Also, please use correct English (capitalization, punctuation, verbs).

In this case, since that was a draft pull request for a while, I think an interactive rebase / history cleanup is needed before merging.

This was also the case with your others PRs. Nothing catastrophic, but we usually prefer clean explicit commit messages (and sometimes we get lazy too, so our commit messages aren't always fantastic...).

Please just take care about that.

Sorry about that, I get your point.

Though, I'm personally used to the squash on merge flow, where commits serve mostly only the in-PR purposes. And actually I was going to mention this issue, too. Why doesn't Picard use squash on merge? The CI/CD works just fine so bisect is rather not a case and I can't think of any other reason not to squash.

[zas]

Why doesn't Picard use squash on merge?

Usually we want to preserve dev history, it is better to have smaller commits, especially when it comes to use bisect to find when an issue was introduced.
Of course, it all depends on the dev process, but small logical units often make sense.
I use to squash commits, doing interactive rebase before the PR is merged, so, at the end, a simple PR can be one commit, but for bigger PRs squashing all may be too much, and having more steps is usually preferred.

Commits that are squashed are typically:

• commits cancelling each other (like added/removed code)
• commits fixing a previous commit (like typos)
• commits fixing a previous commit that broke tests

Each commit is supposed to test successfully, that's not always easy for big feature introduction, but often that's more about code organization: introducing a bunch of methods without their calls (so those are no-op) until everything is in place, then add a commit enabling the added code.

There's nothing written in stone in this matter, but the general rule is:

• small "logic" commits
• clear commit messages
• commit doesn't break tests
• independent changes go in different commits

[zas]

For example, e012e84 should be 2 small commits:

• one introducing _init_remote_commands()
• one changing the loop in get_album_pane_tracks()

Why? Because those are independent changes.

[skelly37]

@zas Okay, understood, thanks for explaining your convention to me.

My suggestion is then to split it into such commits commits:

1. ParseItemsToLoad got prepared to parse commands // all the changes that happened there
2. Method to get all the tracks from album pane for Tagger // get_album_pane_tracks
3. Command handling introduced to Tagger // L389-L441
4. Commands are actually passed via command-line // parser.add_argument and needed changes in the main function

Are you okay with that, assuming that none of you will request changes not related to the git?

[skelly37]

ping @phw

[zas]

LGTM

[skelly37]

Okay, all the ideas implemented, the PR is ready imo.

I'd just like to ask anyone to manually test the lookup_cd command. I have no music CDs here so I cannot actually test if I've rewritten it correctly.

[skelly37]

The one main change that I would like to see is replacing Line 1377 in tagger.py with something like (untested):

                cmd_args = e[1:] if len(e) > 1 else ['']
                for cmd_arg in cmd_args:
                    to_be_added.append("command://{0} {1}".format(e[0], cmd_arg))

to make more effective use of argparse and allow multiple files / arguments for the commands, but this can wait until the next round of changes.

I haven't implemented multiple files support on purpose. File paths can contain whitespaces, so it could lead to an unexpected behavior, as you have used ' ' as a separator. But anyway, \0 is the ONLY correct separator but it's already used as a separator for the pipe messages. I prioritized working correctly in the edge cases over some kind of convenience. Unless you have some better idea or I do not understand something, I'd drop this idea.

[rdswift]

The one main change that I would like to see is replacing Line 1377 in tagger.py with something like (untested):

                cmd_args = e[1:] if len(e) > 1 else ['']
                for cmd_arg in cmd_args:
                    to_be_added.append("command://{0} {1}".format(e[0], cmd_arg))

to make more effective use of argparse and allow multiple files / arguments for the commands, but this can wait until the next round of changes.

I haven't implemented multiple files support on purpose. File paths can contain whitespaces, so it could lead to an unexpected behavior, as you have used ' ' as a separator. But anyway, \0 is the ONLY correct separator but it's already used as a separator for the pipe messages. I prioritized working correctly in the edge cases over some kind of convenience. Unless you have some better idea or I do not understand something, I'd drop this idea.

I must be missing something because I don't understand your reluctance to accept multiple arguments for a command. If you look closely at the code I proposed, it parses the multiple arguments and creates a separate to_be_added item for each argument. This should avoid any issues with trying to separate multiple arguments for passing to the pipe.

[skelly37]

The one main change that I would like to see is replacing Line 1377 in tagger.py with something like (untested):

                cmd_args = e[1:] if len(e) > 1 else ['']
                for cmd_arg in cmd_args:
                    to_be_added.append("command://{0} {1}".format(e[0], cmd_arg))

to make more effective use of argparse and allow multiple files / arguments for the commands, but this can wait until the next round of changes.

I haven't implemented multiple files support on purpose. File paths can contain whitespaces, so it could lead to an unexpected behavior, as you have used ' ' as a separator. But anyway, \0 is the ONLY correct separator but it's already used as a separator for the pipe messages. I prioritized working correctly in the edge cases over some kind of convenience. Unless you have some better idea or I do not understand something, I'd drop this idea.

I must be missing something because I don't understand your reluctance to accept multiple arguments for a command. If you look closely at the code I proposed, it parses the multiple arguments and creats a separate to_be_added item for each argument. This should avoid any issues with trying to separate multiple arguments for passing to the pipe.

picard -e my/fi le.mp3 e xa/mple.mp3 -> How to determine the correct file names via argparse?

[rdswift]

picard -e my/fi le.mp3 e xa/mple.mp3 -> How to determine the correct file names via argparse?

Arguments which include spaces should be quoted. That's standard behavior. For example, they should be entered as:

picard -e LOAD "my/fi le.mp3" "e xa/mple.mp3"

In addition, have you tested your current processing if a file path contains more than one consecutive space? I think you'll find that multiple spaces will be collapsed to a single space. This will not be the case if the arguments containing spaces (single or multiple) are enclosed within quotes.

[skelly37]

Okay, this way. Sorry, my brain got a lag :) Thanks for the suggestion, now I agree it has to be done.