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! |
| 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! |
|
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
|
|
@bal-e sorry I didn't realise this was ready! If you rebase the PR off master the coverage should run fine now. |
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
|
@snejus I tried |
|
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 ( |
.github/workflows/ci.yaml
Outdated
| @@ -1,6 +1,6 @@ | |||
| name: Test | |||
| on: | |||
| pull_request: | |||
| pull_request_target: | |||
There was a problem hiding this comment.
Best to remove this, see my comment for more context
| self.io.restore() | ||
| self.lib._close() | ||
| while len(self.contexts) != 0: |
There was a problem hiding this comment.
TestCase has a method addCleanup which accepts the finalizer function. It can be called in the very beginning, so it should slightly simplify things here. See
def setup_with_context_manager(self, cm):
val = cm.__enter__()
self.addCleanup(cm.__exit__, None, None, None)
return valInterestingly, using addCleanup is recommended over teardown methods, since this makes sure that the cleanup is always performed. On the other hand teardown may not get called if something fails in the setUp method, I think
There was a problem hiding this comment.
I think this should remove the need for the contexts list.
| db_loc: Path | None = None, | ||
| ) -> Iterator[Library]: | ||
| # Create the Beets library object. | ||
| db_loc = util.bytestring_path(db_loc) if db_loc is not None else ":memory:" |
There was a problem hiding this comment.
mypy is failing here because db_loc type becomes bytes | str while Path | None is expected. You can deal with this by moving this logic directly to the Library call:
lib = Library(util.bytestring_path(db_loc) if db_loc is not None else ":memory:", str(lib_dir))There was a problem hiding this comment.
Note that bytestring_path cast may not be required since the default value for path parameter seems to be a string:
class Library(dbcore.Database):
"""A database of music containing songs and albums."""
_models = (Item, Album)
def __init__(
self,
path="library.blb",
directory: str | None = None,
path_formats=((PF_KEY_DEFAULT, "$artist/$album/$track $title"),),
replacements=None,
):| # Set up the basic configuration with a HOME directory. | ||
| self.create_temp_dir() | ||
| temp_dir = Path(os.fsdecode(self.temp_dir)) | ||
| self.libdir = temp_dir / "libdir" |
There was a problem hiding this comment.
self.libdir starts as a Path and later gets cast to bytes. As a Path, it is only used within this method, so you may want to assign a local variable for it, and set self.libdir with bytes later:
libdir = temp_dir / "libdir"
...
self.libdir = util.bytestring_path(libdir)Note the usage of bytestring_path: remember the weird magic/bytes/Windows logic? 😅
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.