lastgenre: Refactor genre applying and pretend mode #6021
Conversation
Reviewer's GuideRefactors genre assignment and pretend-mode handling by extracting common logging/apply logic into a decorator and dedicated methods, updating command and import hooks to use them, auto-enabling force on pretend mode, and streamlining tests with pytest-mock. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as resolved.
This comment was marked as resolved.
JOJ0
changed the title
--pretend option--pretend option, item to album genre fallback
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The fallback-to-album genre behavior is currently hard-coded; consider adding a config option to enable or disable track-level fallback for more flexibility.
- In the track processing loop you check
if item.genre:(the old value) instead of testing the lookup result (item_genre), which can cause incorrect logging or assignment—update that condition to useitem_genre.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fallback-to-album genre behavior is currently hard-coded; consider adding a config option to enable or disable track-level fallback for more flexibility.
- In the track processing loop you check `if item.genre:` (the old value) instead of testing the lookup result (`item_genre`), which can cause incorrect logging or assignment—update that condition to use `item_genre`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Setting this to "ready to review" since I'd like to get this merged before any new feature I have in draft already and to get a sourcery review already.. |
JOJ0
force-pushed
the
lastgenre_pretend
branch
3 times, most recently
from
6adab6b to
097591e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@sourcery-ai guide @sourcery-ai summary |
|
I think it would be a good idea to add |
This comment was marked as duplicate.
This comment was marked as duplicate.
Ah let me reconsider @snejus: I see you already reviewed the pretend PR from @arsaboo, so if you think this is ready to merge soon, please go ahead you both and finishe it! I can work from there an adapt this PR to only make pretend conditional smarter/shorter with my decorator refactor idea here! I think I misunderstood earlier and you ment it that way @snejus right? |
@snejus ? Was this your intention? |
|
@JOJ0 indeed - it's mostly finished so it's we can finish it there and you can base your work on top of it! |
JOJ0
force-pushed
the
lastgenre_pretend
branch
from
097591e to
d7a3a1e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
JOJ0
changed the title
--pretend option, item to album genre fallback
JOJ0
force-pushed
the
lastgenre_pretend
branch
from
d7a3a1e to
5b2cf75
Compare
JOJ0
force-pushed
the
lastgenre_pretend
branch
from
5b2cf75 to
d6da174
Compare
JOJ0
changed the title
JOJ0
force-pushed
the
lastgenre_pretend
branch
from
c92ca25 to
a9f68b9
Compare
|
@sourcery-ai dismiss |
JOJ0
force-pushed
the
lastgenre_pretend
branch
2 times, most recently
from
dd61c89 to
b76cd5b
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `beetsplug/lastgenre/__init__.py:591` </location>
<code_context>
+ item,
+ )
+ else:
+ self._apply_item_genre(item, label, item.genre)
+ if write:
+ item.try_write()
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing item.genre instead of item_genre may result in incorrect genre assignment.
Pass item_genre to _apply_item_genre to ensure the genre field is updated correctly.
</issue_to_address>
### Comment 2
<location> `test/plugins/test_lastgenre.py:175-184` </location>
<code_context>
+def test_pretend_option_skips_library_updates(mocker):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding test coverage for pretend mode with both album and singleton (track) operations.
Please add a test for pretend mode with singleton tracks to verify correct behavior and logging.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JOJ0
force-pushed
the
lastgenre_pretend
branch
from
b76cd5b to
93610e3
Compare
The feature was removed from this PR.
Removed this feature from this PR. Just refactoring of genre applying/pretend handling now.
Terminology changed to "logging label / stage label" a while ago already. This is fine and consistent with other parts of the code. |
- Move item and genre apply to separate helper functions. Have one function for each to not overcomplicate implementation! - Use a decorator log_and_pretend that logs and does the right thing depending on wheter --pretend was passed or not. - Sets --force (internally) automatically if --pretend is given (this is a behavirol change needing discussion)
only one arg is passed to the info log anymore.
JOJ0
force-pushed
the
lastgenre_pretend
branch
from
93610e3 to
aaa9be1
Compare
| assert res == ["ambient", "electronic"] | ||
|
|
||
|
|
||
| def test_pretend_option_skips_library_updates(mocker): |
There was a problem hiding this comment.
This test can be simplified and made more clear by:
-
Keeping the test within the
LastGenrePluginTestto remove the manual need to runsetUpandtearDown(which negatespyetstbenefits). -
Using our
self.run_commandabstraction to run the command. Note you will need to inherit fromPluginTestCaseand definepluginattribute for this:from beets.test.helper import PluginTestCase from beetsplug import lastgenre class LastGenrePluginTest(PluginTestCase): plugin = "lastgenre"
-
Using idiomatic
self.assertLogs()to catch logging output. -
Patching
Item.storewith a function that 'explodes' with a clear message. Note that I think we're supposed to teststorerather thanwrite, I think?
@patch("beets.ui.should_write", Mock(return_value=True))
@patch(
"beetsplug.lastgenre.LastGenrePlugin._get_genre",
Mock(return_value=("Mock Genre", "mock stage")),
)
def test_pretend_option_skips_library_updates(self):
item = self.create_item(
album="Pretend Album",
albumartist="Pretend Artist",
artist="Pretend Artist",
title="Pretend Track",
genre="Original Genre",
)
album = self.lib.add_album([item])
def unexpected_store(*_, **__):
raise AssertionError("Unexpected store call")
# Verify that try_write was never called (file operations skipped)
with (
patch("beetsplug.lastgenre.Item.store", unexpected_store),
self.assertLogs() as logs,
):
self.run_command("lastgenre", "--pretend")
assert "Mock Genre" in str(logs.output)
album.load()
assert album.genre == "Original Genre"
assert album.items()[0].genre == "Original Genre"| write = ui.should_write() | ||
| pretend = getattr(opts, "pretend", False) | ||
| self.config.set_args(opts) | ||
| if opts.pretend: |
| item_genre, label = self._get_genre(item) | ||
|
|
||
| if not item_genre: | ||
| self._log.info( |
There was a problem hiding this comment.
Shouldn't we have the same logic in two other places?
- the singleton track on line 599
- the equivalent operation for album tracks under
imported, line 617
| if write and not pretend: | ||
| item.try_write() | ||
| singleton_genre, label = self._get_genre(item) | ||
| self._apply_item_genre(item, label, singleton_genre) |
There was a problem hiding this comment.
Why are we not writing singleton like we write album items?
| # Store the album genre and inherit to tracks. | ||
| else: | ||
| album.store() | ||
| item_genre, label = self._get_genre(item) |
There was a problem hiding this comment.
To ensure that album tracks and singletons are processed consistently regardless of whether it's run by the command or the import, I'd suggest defining
def _process_item(self, item: Item, write: bool = False):
genre, label = self._get_genre(item)
if genre:
self._apply_item_genre(item, label, genre)
if write and not self.config["pretend"]:
item.try_write()
else:
self._log.info('No genre found for track "{.title}"', item)| if write: | ||
| item.try_write() |
There was a problem hiding this comment.
Seems like files will get written even if --pretend is given?
Maybe it should be
| if write: | |
| item.try_write() | |
| if write and self.config["pretend"]: | |
| item.try_write() |
| 'genre for album "{0.album}" ({1}): {0.genre}', album, src | ||
| ) | ||
| album_genre, label = self._get_genre(album) | ||
| self._apply_album_genre(album, label, album_genre) |
There was a problem hiding this comment.
Seems like album logic is the same in lastgenre_func and imported, so you can probably define
def _process_album(self, album: Album, write: bool = False):
album_genre, label = self._get_genre(album)
self._apply_album_genre(album, label, album_genre)
# If we're using track-level sources, store the album genre only (this
# happened in _apply_album_genre already), then also look up individual
# track genres.
if "track" in self.sources:
for item in album.items():
self._process_item(item, write=write)
Description
Shorten final album/genre-apply code. This was longish code already and since the
--pretendoption is here it is even longer, thus I'd like to smarten/shorten that. Also behavior is changed so that--pretendis automatically enabling force mode (-por-fis what the user usually expects to being the opposite of each other, but we can discuss that of course)To Do
Documentation.Not required. No behavioral changes