tidal: Normalize copyright text into a concise label name - #6907
tidal: Normalize copyright text into a concise label name#6907NoDancing wants to merge 3 commits into
Conversation
_parse_label() used to store Tidal's raw copyright info as the label name. Add _normalize_label(), a helper that strips leading markers, years, legal entities, and boilerplate clauses. Fixes beetbox#6796
There was a problem hiding this comment.
Pull request overview
PR make Tidal plugin stop stuffing big rights-statement blob into label. PR add helper to shave blob down to small label name, so autotag/import metadata look sane.
Changes:
- Add
TidalPlugin._normalize_label()to strip marker/year and trim known rights boilerplate from Tidal copyright text. - Update
_parse_label()to return normalized label instead of rawcopyright.text. - Add parametrized tests + changelog entry for bug #6796.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
beetsplug/tidal/__init__.py |
Add label-normalize helper + route _parse_label() through it. |
test/plugins/test_tidal.py |
Update label parsing expectation + add normalization test matrix. |
docs/changelog.rst |
Document behavior change for label field in Tidal plugin. |
Suppressed comments (2)
beetsplug/tidal/init.py:426
- grug see year-strip run always, even when no ©/℗/(C)/(P) marker. then label like "2001 Records" get head chopped. issue ask year remove only after marker, be conservative.
text = TidalPlugin._LEADING_MARKER_RE.sub("", text, count=1)
text = TidalPlugin._LEADING_YEAR_RE.sub("", text, count=1)
beetsplug/tidal/init.py:432
- grug see corporate trim happen before "under exclusive license to" keep-right-side rule. if text like "Foo, a Division of Bar under exclusive license to Baz", current code cut at ", a" and never keep Baz. do license split first, then trim rest.
if match := TidalPlugin._CORPORATE_RE.search(text):
text = text[: match.start()]
if match := TidalPlugin._LICENSE_TO_RE.search(text):
text = text[match.end() :]
| """Removes leading copyright markers, years, and trims corporate boilerplate | ||
| from Tidal copyright text. | ||
| """ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6907 +/- ##
==========================================
+ Coverage 75.69% 75.72% +0.02%
==========================================
Files 163 163
Lines 21412 21442 +30
Branches 3384 3393 +9
==========================================
+ Hits 16208 16236 +28
- Misses 4405 4406 +1
- Partials 799 800 +1
🚀 New features to boost your workflow:
|
| def _parse_label(attributes: MediaAttributes) -> str | None: | ||
| if copyright_ := attributes.get("copyright"): | ||
| return copyright_["text"] | ||
| return TidalPlugin._normalize_label(copyright_["text"]) |
There was a problem hiding this comment.
How about we skip the regex patterns entirely? I try to omit regex whenever possible for readability and performance reasons.
@staticmethod
def _normalize_label(text: str) -> str:
text = text.strip()
# Leading copyright markers
for marker in ("©", "℗", "(C)", "(P)", "(c)", "(p)"):
if text.startswith(marker):
text = text[len(marker):].strip()
break
# Leading year
if len(text) >= 4 and text[:4].isdigit():
text = text[4:].lstrip()
# ", a "
lower = text.lower()
if ", a " in lower:
idx = lower.index(", a ")
text = text[:idx]
# "under exclusive license to"
phrase = "under exclusive license to"
phrase_alt = "under exclusive licence to"
lower = text.lower()
for p in (phrase, phrase_alt):
if p in lower:
idx = lower.index(p)
text = text[idx + len(p):]
break
# Trailing company suffixes
for suffix in (" inc.", " inc", " llc", " ltd.", " ltd", " co.", " co"):
if text.lower().endswith(suffix):
text = text[:-len(suffix)].rstrip(", ")
break
# Territorial boilerplate
phrase = " for the united states and "
lower = text.lower()
if phrase in lower:
text = text[:lower.index(phrase)]
return text.strip()There was a problem hiding this comment.
@semohr in my opinion regex patterns are simpler and more readable than the above.
Replace the regex-based function with string methods, per review feedback. Also fixes a potential bug with the order of the license-to and the corporate-relationship clause. Verified against the same copyright samples as the previous implementation, output is identical.
|
Alright! Implemented those changes, nearly exactly as your wrote. I just added the docstring back in, changed the ordering of the corporate relationship and the license-to parts, (since one could cancel out the other), and I made sure there's a space between the digits and the next word for the year. |
There was a problem hiding this comment.
You can simplify this to:
diff --git a/beetsplug/_utils/func.py b/beetsplug/_utils/func.py
new file mode 100644
index 000000000..4907702d3
--- /dev/null
+++ b/beetsplug/_utils/func.py
@@ -0,0 +1,13 @@
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from collections.abc import Callable, Iterable
+
+
+def apply_transforms(text: str, methods: Iterable[Callable[[str], str]]) -> str:
+ for method in methods:
+ text = method(text)
+
+ return text
diff --git a/beetsplug/lyrics.py b/beetsplug/lyrics.py
index c9e5c529d..4f10e4f70 100644
--- a/beetsplug/lyrics.py
+++ b/beetsplug/lyrics.py
@@ -28,6 +28,7 @@
from beets.util.config import sanitize_choices
from beets.util.lyrics import INSTRUMENTAL_LYRICS, Lyrics
+from ._utils.func import apply_transforms
from ._utils.requests import (
HTTPNotFoundError,
RequestHandler,
@@ -35,7 +36,7 @@
)
if TYPE_CHECKING:
- from collections.abc import Callable, Iterable, Iterator
+ from collections.abc import Iterable, Iterator
import confuse
@@ -51,8 +52,6 @@
TranslatorAPI,
)
- HtmlTransformer = Callable[[str], str]
-
class CaptchaError(requests.exceptions.HTTPError):
def __init__(self, *args, **kwargs) -> None:
@@ -441,13 +440,6 @@ def fetch
return Lyrics(lyrics, self.__class__.name, url)
-def apply_transforms(html: str, methods: Iterable[HtmlTransformer]) -> str:
- for method in methods:
- html = method(html)
-
- return html
-
-
class Html:
collapse_space = partial(re.compile(r"(^| ) +", re.M).sub, r"\1")
expand_br = partial(re.compile(r"\s*<br[^>]*>\s*", re.I).sub, "\n")
diff --git a/beetsplug/tidal/__init__.py b/beetsplug/tidal/__init__.py
index 1a7091ef5..a2532ebff 100644
--- a/beetsplug/tidal/__init__.py
+++ b/beetsplug/tidal/__init__.py
@@ -4,7 +4,7 @@
import os
import re
import time
-from functools import cached_property
+from functools import cached_property, partial
from typing import TYPE_CHECKING, ClassVar, Literal, overload
import confuse
@@ -15,6 +15,7 @@
from beets.exceptions import UserError
from beets.logging import getLogger
from beets.metadata_plugins import MetadataSourcePlugin
+from beetsplug._utils.func import apply_transforms
from .api import TidalAPI
@@ -40,6 +41,19 @@
log = getLogger("beets.tidal")
+_remove_leading = partial(
+ re.compile(r"^(?:(?:©|℗|\([cp]\)) *)?(?:\d{4} )?", re.IGNORECASE).sub, ""
+)
+_remove_license = partial(
+ re.compile(r".* under exclusive licence to ", re.IGNORECASE).sub, ""
+)
+_remove_trailing_clause = partial(
+ re.compile(r"(?:, a| for the united states and) .*", re.IGNORECASE).sub, ""
+)
+_remove_legal_suffix = partial(
+ re.compile(r",? (?:inc|llc|ltd|co)\.?$", re.IGNORECASE).sub, ""
+)
+
class TidalPlugin(MetadataSourcePlugin):
item_types: ClassVar[dict[str, types.Type]] = {
@@ -394,50 +408,15 @@ def _get_album_info
@staticmethod
def _normalize_label(text: str) -> str:
- """Normalize label from Tidal copyright text by stripping markers/years
- and known corporate/licensing/territorial boilerplate."""
- text = text.strip()
-
- # Leading copyright markers
- for marker in ("©", "℗", "(C)", "(P)", "(c)", "(p)"):
- if text.startswith(marker):
- text = text[len(marker) :].strip()
- break
-
- # Leading year
- first, _, rest = text.partition(" ")
- if len(first) == 4 and first.isdigit():
- text = rest.lstrip()
-
- # "under exclusive license to"
- phrase = "under exclusive license to"
- phrase_alt = "under exclusive licence to"
-
- lower = text.lower()
- for p in (phrase, phrase_alt):
- if p in lower:
- idx = lower.index(p)
- text = text[idx + len(p) :]
- break
-
- # ", a "
- lower = text.lower()
- if ", a " in lower:
- text = text[: lower.index(", a ")]
-
- # Territorial boilerplate
- phrase = " for the united states and "
- lower = text.lower()
- if phrase in lower:
- text = text[: lower.index(phrase)]
-
- # Trailing company suffixes
- for suffix in (" inc.", " inc", " llc", " ltd.", " ltd", " co.", " co"):
- if text.lower().endswith(suffix):
- text = text[: -len(suffix)].rstrip(", ")
- break
-
- return text.strip()
+ return apply_transforms(
+ text,
+ [
+ _remove_leading,
+ _remove_license,
+ _remove_trailing_clause,
+ _remove_legal_suffix,
+ ],
+ )
@staticmethod
def _parse_artwork_url(Note: you will want to move apply_transforms from beetsplug/lyrics.py to a new module beetsplug/_utils/func.py.
Return _normalize_label to regex-based logic, per review. Uses the apply_transforms method that was previously in lyrics.py, now moved to new module beetsplug/_utis/func.py.
There was a problem hiding this comment.
I would keep the non-regex version. The logic here is straightforward, and I don't think it's worth compiling four regex patterns just because we import the Tidal plugin.
For example, to understand this:
partial(
re.compile(r"^(?:(?:©|℗|\([cp]\)) *)?(?:\d{4} )?", re.IGNORECASE).sub, ""
)a reader needs to:
- understand the regex itself,
- know what re.compile() is doing,
- and know why functools.partial() is being used.
That's a fair amount of indirection for logic that is otherwise very simple.
The explicit version:
for marker in ("©", "℗", "(C)", "(P)", "(c)", "(p)"):
if text.startswith(marker):
text = text[len(marker):].strip()
breakis immediately obvious to most Python developers, even those who aren't comfortable with regexes. It also avoids the overhead of compiling regex patterns, so it's slightly more efficient as well.
Unless the matching logic becomes substantially more complex, I'd favor the simpler, more explicit implementation.
If we combine all patterns into a single compiled regex, the performance argument becomes less relevant.
For me, the stronger argument remains that the explicit implementation communicates the intent more clearly. Unless the matching rules become substantially more complex, I would favor the simpler, more direct approach.
Regarding this topic, I recommend https://dl.acm.org/doi/abs/10.1109/ASE.2019.00047
|
I'll hold off on making any more changes until you come to a consensus. Thanks for sharing that research paper! Good perspective, there's stuff in there I genuinely didn't know. |
Thanks for expanding on this. I agree that the previous combined _remove_copyright = partial(
re.compile(r"^(©|℗|\([cp]\)) *", re.IGNORECASE).sub, ""
)
_remove_year = partial(re.compile(r"^\d{4} ", re.IGNORECASE).sub, "")With that change, I find the complete regex version easier to follow than the string version. The normalization policy is visible as one short pipeline: [
_remove_copyright,
_remove_year,
_remove_license,
_remove_trailing_clause,
_remove_legal_suffix,
]Each regex now has one narrowly named responsibility. They also use only basic regex features that developers should be familiar with. By comparison, the string version spreads the same policy across repeated
I don't think performance should decide this either way. I read the paper you linked. It supports caution around complex regexes, but it also reports that developers choose regexes for medium-complexity matching and prefer shorter expressions with fewer features. I think these small, named, tested transformations fit that use case better than one large imperative function. |
Description
Fixes #6796
_parse_label()used to store Tidal's raw copyright info as the label name.Adds
_normalize_label(), a single-pass helper that:©/℗/(C)/(P)marker and year, a Division of X,, a BMG Company)under exclusive license to XstatementsX for the United States and Y for the world outside...), keeping the first labelInc.,LLC,Ltd.,Co.)Tests show all functionality in action, but for the curious, here's the results of running the helper function on a random selection of albums: https://gist.github.com/NoDancing/90fae264e46023de176a11cb5c065a58
To Do