Rewrite the fish plugin from scratch
#5359
Conversation
|
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. |
|
Fish's |
|
cc @Serene-Arc, this follows on from the small changes I made to |
There was a problem hiding this comment.
I'm not familiar with fish but the code looks fine to me. One thing that seems absolutely paramount is tests though. If I can't understand everything that is being done, I'd advocate for tests normally but here it's paramount.
When I look at this, a programmatically generated shell script that is meant to be autoloaded, that screams 'attack vector' to me. At the very least, we need tests that take the generated script, write it to a file, and then the test compares that written script to one we have a string in the document. That will make it crystal clear to developers (and users, if they are inclined to look) what exactly all this code generates, and what changes with each option. Plus, if someone changes the logic, they'll need to change the tests, which will show exactly what will change without any obfuscation.
|
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. |
|
@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 |
|
@snejus, I know you were working on the testing infrastructure in #5362. I've ended up getting started on the |
|
Sorry - my CLI has gone haywire and accidentally approved this PR 😅 |
|
no worries @snejus! |
|
@snejus bump on review? |
|
Ah sorry, completely missed out on this! Will have a look now! |
|
|
||
|
|
||
| @pytest.fixture | ||
| def config( |
There was a problem hiding this comment.
I'm a bit concerned about multiple sources of truth for the same beets config setup in setup_beets and this fixture. If these two configurations get out of sync, we are more likely to see unexpected test failures attempting to port tests from unittest to pytest. This can be a nightmare to debug due to the lack of clear indication of the root cause.
What would you think about defining the logic in your config fixture as a context manager in beets.test.helper? This way we could reuse it in both BeetsTestCase and pytest fixtures, and remove the duplicate implementation from TestHelper.setup_beets.
For example
# beets/test/helper.py
@contextmanager
def setup_config(directory: Path):
config = beets.config
config.clear()
config.read()
config["plugins"] = []
config["verbose"] = 1
config["ui"]["color"] = False
config["threaded"] = False
config["directory"] = str(tmp_path)
yield config
self.config.clear()
beets.config.read(user=False, defaults=True)It encapsulates all setup and cleanup logic and it can be used both in the setUp code and pytest fixtures:
# beets/test/helper.py
class TestHelper:
...
def setup_with_context_manager(self, cm):
val = cm.__enter__()
self.addCleanup(cm.__exit__, None, None, None)
return val
def setUp(self):
...
self.config = self.setup_with_context_manager(setup_config(self.tmp_path))
...
# conftest.py
from beets.test.helper import setup_config
@pytest.fixture
def config(tmp_path):
with setup_config(tmp_path) as config:
yield configThis way we keep a single source of truth for the beets config setup, maintaining consistency regardless of the test runner. This pattern is also applicable to the rest of the setup (lib, importer etc.). What do you think?
A nice side effect of this approach is that we can use conftest.py to define our fixtures since the stuff useful to people is defined in beets.test.helper and not in the test case itself. This makes the test cases more readable and easier to maintain.
There was a problem hiding this comment.
I like this plan, thanks! Once we're done with unittest we can revert back to the existing layout, though. I'll leave a TODO in the code for that future change.
| The directory for the user's Fish configuration. | ||
| """ | ||
|
|
||
| config_home: Path |
There was a problem hiding this comment.
Oh, shoot. I completely forgot about the platformdirs PR I was supposed to open. Do you mind if I leave this as-is for now and address it in that PR?
There was a problem hiding this comment.
Sounds good. You may be able to drop this altogether if you decide to print the completions to stdout instead of saving them to a file 😉
bal-e
force-pushed
the
rewrite-fish
branch
2 times, most recently
from
79d248f
to
9e0c504
Compare
|
It looks like the Windows builds are broken because of the |
You can now rebase as the fix has been merged upstream 🙂 |
| @@ -0,0 +1,71 @@ | |||
|
|
|||
| # An 'argparse' description for global options for 'beet'. | |||
| function __fish_beet_global_optspec | |||
There was a problem hiding this comment.
I was meant to add a comment to ask to move this to a .fish file but forgot. Happy to see that it was done nevertheless!
|
Everything looks good - there's only one bit that's left, see #5359 (review). What did we decide to do about the test setup? |
|
I'll implement it now, @snejus! |
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.
This allows us to test that the generated script is exactly the same as a pre-set file.
This feature was added in 3.9; instead, we fall back to constructing a dict from an iterable.
For Python 3.8 support.
Apparently the 'Iterator' in 'collections.abc' did not support generics, at least not in 3.8.
- Don't explicitly state 'scope="function"'. - Use 'tmp_path_factory' instead of doing it manually. - Set up 'conftest.py' for importing fixtures automatically.
- Fixed a 'mypy' error regarding 'set' - Print an error if unnecessary arguments are added
This plugin needed significant reworking. It now uses
StringIOto construct the completion script andpathlibto 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.