beets Rewrite the `fish` plugin from scratch

This plugin needed significant reworking. It now uses StringIO to construct the completion script and pathlib to manage paths uniformly. Functions now have type annotations and the flow of information through the entire script is much more readable.

More importantly, the actual contents of the script produced have been rewritten entirely. A more sophisticated parsing function is now used to determine when to show global option completions. Based on Fish's source code, a much more comprehensive string escaping function is now used to put data in the completion script. The completion for metadata values (as provided by --extravalues) actually works now: it needed to provide completions for the word the user was in the middle of typing (e.g. artist:ab -> artist:abba), rather than the word following.

[x] Documentation
[ ] Changelog
[ ] Tests

Jul 11 '24 20:07 bal-e

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Jul 11 '24 20:07 github-actions[bot]

Fish's complete builtin offers a -C flag which lets you test what completions would be offered for a certain command-line; this could be used to perform automated testing. I suppose this is doable, by making any such tests conditional on a Fish executable being found. But I don't know whether that should be a separate PR or not. This one is already a complete rewrite of the plugin.

Jul 11 '24 20:07 bal-e

cc @Serene-Arc, this follows on from the small changes I made to fish.py in #5337. The more I looked at the code, the more I wanted to rewrite it.

Jul 11 '24 21:07 bal-e

I agree, a comprehensive testing solution is important. I was also concerned about the security of these sorts of shell scripts, but I hadn't thought about just comparing the results to a fixed script string in the tests -- I'll definitely implement that.

Jul 18 '24 05:07 bal-e

@Serene-Arc, I've implemented a basic file test, so there is some guarantee that the generated script will not contain malware. I can cover some more cases (e.g. using sample items from the library and checking the script with --extravalues) here, but I'd like to address them and an actual testing solution (where a Fish instance is launched and the generated script is tested) in a later PR. Let me know what you'd prefer.

Jul 22 '24 18:07 bal-e

@snejus, I know you were working on the testing infrastructure in #5362. I've ended up getting started on the pytest migration by defining some shared fixtures here, based on the code from your PR -- can you take a look (at beets/test/fixtures.py in particular) to make sure it all checks out? It can coexist with the existing testing infrastructure so we can migrate over time.

Jul 27 '24 13:07 bal-e

Sorry - my CLI has gone haywire and accidentally approved this PR 😅

Jul 28 '24 18:07 snejus

no worries @snejus!

Jul 28 '24 20:07 bal-e

@snejus bump on review?

Aug 19 '24 19:08 bal-e

Ah sorry, completely missed out on this! Will have a look now!

Aug 19 '24 19:08 snejus

It looks like the Windows builds are broken because of the reflink issue. @snejus, feel free to review anyway.

Sep 04 '24 13:09 bal-e

It looks like the Windows builds are broken because of the reflink issue. @snejus, feel free to review anyway.

You can now rebase as the fix has been merged upstream 🙂

Sep 12 '24 11:09 snejus

Everything looks good - there's only one bit that's left, see https://github.com/beetbox/beets/pull/5359#pullrequestreview-2246989763. What did we decide to do about the test setup?

Sep 17 '24 15:09 snejus

I'll implement it now, @snejus!

Sep 19 '24 19:09 bal-e

There seems to be some weird issue with test coverage. @snejus, any idea what's going on?

Oct 12 '24 21:10 bal-e

There seems to be some weird issue with test coverage. @snejus, any idea what's going on?

I have a feeling that's because the workflow is being run from your fork, where the environmental variable doesn't exist.

Try replacing pull_request with pull_request_target in .github/workflows/ci.yaml?

Oct 13 '24 13:10 snejus

@bal-e sorry I didn't realise this was ready! If you rebase the PR off master the coverage should run fine now.

Oct 27 '24 00:10 snejus

@snejus I tried pull_request_target, but it looks like that disabled CI checking for the PR entirely. Is there any good way to disable specifically the upload-coverage action for PRs?

Nov 12 '24 17:11 bal-e

@bal-e see #5479 where I did the same change which later got reverted since I discovered it indeed disabled PR checks 😅

Is there any good way to disable specifically the upload-coverage action for PRs?

Haven't yet looked into this... For now, I guess, you can ignore this failure

Nov 13 '24 14:11 snejus

For the future PRs, note that since you're a member of the organization you can push your work directly to a branch in this repo (beetbox/beets) - this issue does not come up. That's what I've been doing myself.

Nov 13 '24 14:11 snejus

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Apr 26 '25 01:04 stale[bot]