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

fromfilename: Fix regex ordering and add debug logging #4600

Merged
merged 5 commits into from
Jan 2, 2023

Conversation

JOJ0
Copy link
Member

@JOJ0 JOJ0 commented Dec 22, 2022

Description

To Do

  • Documentation.
  • Changelog.
  • Tests.

@wisp3rwind
Copy link
Member

Thanks for getting this started! As you already noticed in #4561, this pattern should really go to the very end of the list, since it's the least specific.

@wisp3rwind
Copy link
Member

LGTM; any reason why this is still marked as a draft?

@JOJ0
Copy link
Member Author

JOJ0 commented Dec 28, 2022

LGTM; any reason why this is still marked as a draft?

I'm not sure if the debug log messages with a simple "textual" prefix of fromfilename: are the correct way of doing it. Usually plugins being a child of BeetsPlug, have a nice logging magic built-in: https://github.com/beetbox/beets/blob/master/beets/plugins.py#L88-L91

Implementing this would be quite some hassle because the method the logging takes places in my PR is not part of the plugin. If you think it's sufficient like this and not worth the reafactoring hassle I'm very fine with it.

Also: I'll add a quick changelog entry.

@JOJ0 JOJ0 changed the title fromfilename: Swap regex lines as suggested in #4561 fromfilename: Fix regex ordering and add debug logging Dec 28, 2022
JOJ0 added a commit to JOJ0/beets that referenced this pull request Dec 28, 2022
@JOJ0 JOJ0 marked this pull request as ready for review December 28, 2022 10:44
@wisp3rwind
Copy link
Member

LGTM; any reason why this is still marked as a draft?

I'm not sure if the debug log messages with a simple "textual" prefix of fromfilename: are the correct way of doing it. Usually plugins being a child of BeetsPlug, have a nice logging magic built-in: https://github.com/beetbox/beets/blob/master/beets/plugins.py#L88-L91

Implementing this would be quite some hassle because the method the logging takes places in my PR is not part of the plugin. If you think it's sufficient like this and not worth the reafactoring hassle I'm very fine with it.

You're right, that would be much cleaner to do, thanks for thinking ahead! I think it's not a lot of work actually: Make filename_task a method of FromFilenamePlugin (i.e. indent it and add the self parameter), then pass the self._log logger to apply_matches. There are many plugins that do similar things, for example fetchart.

Would you mind doing that change?

Also: I'll add a quick changelog entry.

👍

@JOJ0
Copy link
Member Author

JOJ0 commented Dec 28, 2022

Sure thing. Will do tomorrow or soonish! Appreciate your hints.

@JOJ0
Copy link
Member Author

JOJ0 commented Dec 31, 2022

Hi, changes as suggested. Tested a little and works well. Also changed log level to INFO. I figured that it should be enough to pass one -v to see the messages. Is there any case for this plugin where INFO level could be inappropriate?

There seems to be an issue with the CI. Could be something with Sphinx build. I'm not sure if it's my fault.

The only thing I did which was Sphinx related was the changelog. I reverted that commit to see if the error persists. Turns out it does.

Maybe something is broken with the current Sphinx version or even something with the Python install process broken on github's end? Not sure but please look into the CI logs. Thanks a lot! I'll continue work on this PR earliest on the 3rd of Jan.

JOJ0 added a commit to JOJ0/beets that referenced this pull request Dec 31, 2022
This reverts commit f223fae.

Test if CI errors are related to changes in this commit.
@JOJ0 JOJ0 marked this pull request as draft December 31, 2022 08:11
@wisp3rwind
Copy link
Member

Hi, changes as suggested. Tested a little and works well. Also changed log level to INFO. I figured that it should be enough to pass one -v to see the messages. Is there any case for this plugin where INFO level could be inappropriate?

Looks good, our default log level for plugin stages appears to be WARNING, so INFO wont show up by default.

There seems to be an issue with the CI. Could be something with Sphinx build. I'm not sure if it's my fault.

The only thing I did which was Sphinx related was the changelog. I reverted that commit to see if the error persists. Turns out it does.

Weird, I don't immediately have a glue what might be going on here, the CI logs do seem to point towards an issue with Sphinx.

Maybe something is broken with the current Sphinx version or even something with the Python install process broken on github's end? Not sure but please look into the CI logs. Thanks a lot! I'll continue work on this PR earliest on the 3rd of Jan.

@jackwilsdon
Copy link
Member

I've fixed the Sphinx issue in 2106f47 - if you update your branch from master then you should be good 👍

since it's the least significant as discussed in the PR's thread.
that inform when the plugin replaced bad artist, title or tracknumber metadata.
@JOJ0 JOJ0 marked this pull request as ready for review January 2, 2023 12:39
@JOJ0
Copy link
Member Author

JOJ0 commented Jan 2, 2023

Thanks for the fix @jackwilsdon. Ready to review again @wisp3rwind.

@wisp3rwind wisp3rwind merged commit 3ddaba4 into beetbox:master Jan 2, 2023
@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2023

Nicely done, all! And great work tracking down that Sphinx version problem, @jackwilsdon!!!

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.

fromfilename: Fails to detect <track> <title> filename patterns
4 participants