Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add override_album_path_config hook #281

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ali-ramadhan
Copy link
Contributor

@ali-ramadhan ali-ramadhan commented Oct 17, 2024

  • I have read the contributing guide in the documentation.

Description

This PR adds a new hook to the move plugin called override_album_path_config which allows plugins to modify the album path configuration dynamically based on the album's properties.

For more context, see: https://github.com/orgs/MoeMusic/discussions/231

TODO:

  • Add some docs in move.rst.

Additional information

@jtpavlock I took a stab at implementing the override_album_path_config hook we discussed and added some tests which pass locally! But I'm not totally sure that I adhered to moe's design/coding philosophy as I'm still familiarizing myself with the code base. Would appreciate any feedback!


📚 Documentation preview 📚: https://mrmoe--281.org.readthedocs.build/en/281/

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.20%. Comparing base (7dea2e3) to head (daf0fba).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #281   +/-   ##
=======================================
  Coverage   98.20%   98.20%           
=======================================
  Files          37       37           
  Lines        2114     2121    +7     
=======================================
+ Hits         2076     2083    +7     
  Misses         38       38           

@ali-ramadhan
Copy link
Contributor Author

Ah woops I forgot to follow the commit message guidelines. I should be able to rewrite them on this branch.

Copy link
Member

@jtpavlock jtpavlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I'm especially impressed with the tests, I know it probably wasn't easy trying to make sense of how I've structured my testing. I think just a few minor changes, and then clean up the commits and it should be good to go.

For the commit, just a single feature commit would be perfect, no need to have a separate one for tests or anything.

assert path == expected_path

@pytest.mark.usefixtures("_tmp_album_path_config")
def test_soundtrack_album(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really this test (and the soundtrack logic) isn't necessary at all. You only need to test the three cases:

  1. It returns a string which should override the config.
  2. It returns None, and the user config should be used.
  3. No plugin implementation, and the user config should be used.

1 is covered by the classical album logic, and 2 is covered by your test_other_genre_album, and 3 is covered by test_no_plugin.

@@ -1,13 +1,17 @@
"""Tests the core api for moving items."""

import datetime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any of the datetime functionality is actually necessary for any of the tests. Everything should still work if you just remove all date creations and any references to years or dates in the album paths.

@@ -37,6 +37,22 @@ def create_path_template_func() -> list[Callable]: # type: ignore
contain the callable functions themselves.
""" # noqa: DAR202

@staticmethod
@moe.hookspec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to utilize the first-result-only feature of pluggy since there can only be one path template. This also means you can get rid of the "Ensure album_path_config is a string" logic since it will return the result as a string instead of a list.

"""Override the album path configuration.

This hook allows plugins to modify the album path configuration dynamically
based on the album's properties.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it isn't quite clear how the new hook differentiates from create_path_template_func based on this description.

Maybe "allows plugins to programmatically override the user's album path configuration"?

I also think an example would also be useful. The syntax for an example in a docstring is:

        Example:
            .. code:: python

                settings.validators.register(
                    dynaconf.Validator("MOVE.ASCIIFY_PATHS", must_exist=True)
                )

The classical/soundtrack function you use in tests may be a perfect example.


Returns:
An optional string representing the new album path configuration.
If None is returned, the default configuration will be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "default configuration" may imply Moe's default configuration, which isn't the case, but rather whatever is specified in the user's configuration file will be used.

return "Classical/{album.artist}/{album.title} ({album.year})"
elif "Soundtrack" in album.title:
return "Soundtracks/{album.title} ({album.year})"
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you mean to explicitly return None for clarity purposes, but it's not necessary in Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants