Mirrors/kleinanzeigen-bot
1
0
Fork 0
mirror of https://github.com/Second-Hand-Friends/kleinanzeigen-bot.git synced 2026-07-09 12:41:05 +02:00
Code Issues Packages Projects Releases Wiki Activity

refactor: extract selector and filter helpers from load_ads (#1169)

Browse Source 
## ℹ️ Description

Extract three private helpers from `load_ads()` to remove the last
remaining `# noqa: PLR0915` annotation in main source. `load_ads` goes
from ~100 statements to ~35 — a clean orchestration function.

## 📋 Changes Summary

- **`_parse_ad_selector`**: parse `ads_selector` string into numeric IDs
list or frozenset of tokens
- **`_should_include_ad`**: token-based ad filtering
("changed"/"new"/"due"/"all") — preserves exact ordering, side effects,
and "changed"/price-reduction-for-update logic
- **`_prepare_selected_ad_entry`**: description validation, category
resolution, image resolution — called only after filtering to avoid
validating skipped ads
- Translation keys moved to match new caller function names
(`_should_include_ad:`, `_prepare_selected_ad_entry:`)
- Added regression test: inactive ad with unresolvable images is skipped
without error

### ⚙️ Type of Change
- [x] ✨ New feature (adds new functionality without breaking existing
usage)

## ✅ Checklist
- [x] I have reviewed my changes to ensure they meet the project's
standards.
- [x] I have tested my changes and ensured that all tests pass (`pdm run
test`).
- [x] I have formatted the code (`pdm run format`).
- [x] I have verified that linting passes (`pdm run lint`).
- [x] I have updated documentation where necessary.

By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Refactor**
* Reorganized ad loading process with improved selector parsing and
filtering logic for better code maintainability.
* Optimized validation workflow: inactive and non-selected ads now skip
description and image validation, improving overall efficiency.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Jens
2026-06-22 16:07:04 +02:00
committed by GitHub
parent 388d0fb3c8
commit 1f142f8339
3 changed files with 142 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

148
src/kleinanzeigen_bot/ad_loading.py
View File

@@ -245,7 +245,94 @@ def resolve_ad_images(ad_file:str, image_patterns:list[str]) -> list[str]:
# --------------------------------------------------------------------------- #
def load_ads( # noqa: PLR0915
# Selector parsing
def _parse_ad_selector(ads_selector:str) -> tuple[list[int] | None, frozenset[str]]:
"""Parse *ads_selector* into numeric IDs and filter tokens.
Returns:
``(ids, tokens)`` — a list of numeric IDs and a frozenset of
token strings. When the selector is purely numeric, *ids* is the
parsed list and *tokens* is empty. Otherwise *ids* is ``None``
and *tokens* contains the stripped, non-empty selector tokens.
"""
if _download_selection.is_numeric_ids_selector(ads_selector):
return [int(n) for n in ads_selector.split(",")], frozenset()
tokens = frozenset(token.strip() for token in ads_selector.split(",") if token.strip())
return None, tokens
# Selector filtering
def _should_include_ad(
ad_cfg:Ad,
ad_cfg_orig:dict[str, Any],
ad_file_relative:str,
tokens:frozenset[str],
command:str,
*,
exclude_ads_with_id:bool = True,
) -> bool:
"""Return ``True`` when *ad_cfg* matches the given filter *tokens*.
Side effects: ``check_ad_changed`` may mutate ``ad_cfg_orig["content_hash"]``
and may emit log messages.
"""
should_include = False
if "changed" in tokens and check_ad_changed(ad_cfg, ad_cfg_orig, ad_file_relative):
should_include = True
elif "changed" in tokens and command == "update" and _price_reduction.is_auto_price_reduction_due(ad_cfg, ad_file_relative):
# Only the "update" command considers pending price reductions
# as a reason to include a "changed" ad.
should_include = True
if "new" in tokens and (not ad_cfg.id or not exclude_ads_with_id):
should_include = True
elif "new" in tokens and ad_cfg.id and exclude_ads_with_id:
LOG.info(" -> SKIPPED: ad [%s] is not new. already has an id assigned.", ad_file_relative)
if "due" in tokens:
if check_ad_republication(ad_cfg, ad_file_relative):
should_include = True
if "all" in tokens:
should_include = True
return should_include
# Selected-ad enrichment
def _prepare_selected_ad_entry(
ad_file:str,
ad_cfg:Ad,
ad_defaults:Any,
categories:dict[str, str],
) -> None:
"""Validate description, resolve category, and resolve images for an ad
that has already passed inactive and selector filtering.
May mutate *ad_cfg* (category resolution, image list assignment) and
may raise :class:`AssertionError` via :func:`ensure`.
"""
ensure(
get_ad_description(ad_cfg, ad_defaults, with_affixes = False),
_("-> property [description] not specified @ [%s]") % ad_file,
)
get_ad_description(ad_cfg, ad_defaults, with_affixes = True) # validates complete description
resolve_ad_category(ad_cfg, categories)
if ad_cfg.images:
ad_cfg.images = resolve_ad_images(ad_file, ad_cfg.images)
def load_ads(
*,
config_file_path:str,
ad_file_patterns:list[str],
@@ -272,16 +359,10 @@ def load_ads( # noqa: PLR0915
if not ad_files:
return []
ids = []
use_specific_ads = False
# Strip whitespace from selector tokens to match is_valid_ads_selector.
selectors = [token.strip() for token in ads_selector.split(",") if token.strip()]
if _download_selection.is_numeric_ids_selector(ads_selector):
ids = [int(n) for n in ads_selector.split(",")]
use_specific_ads = True
ids, tokens = _parse_ad_selector(ads_selector)
if ids is not None:
LOG.info("Start fetch task for the ad(s) with id(s):")
LOG.info(" | ".join([str(id_) for id_ in ids]))
LOG.info(" | ".join(str(id_) for id_ in ids))
ads:list[tuple[str, Ad, dict[str, Any]]] = []
for ad_file, ad_file_relative in sorted(ad_files.items()):
@@ -294,50 +375,17 @@ def load_ads( # noqa: PLR0915
LOG.info(" -> SKIPPED: inactive ad [%s]", ad_file_relative)
continue
if use_specific_ads:
if ids is not None:
if ad_cfg.id not in ids:
LOG.info(" -> SKIPPED: ad [%s] is not in list of given ids.", ad_file_relative)
continue
else:
# Check if ad should be included based on selectors
should_include = False
elif not _should_include_ad(
ad_cfg, ad_cfg_orig, ad_file_relative,
tokens, command, exclude_ads_with_id = exclude_ads_with_id,
):
continue
# Check for 'changed' selector
if "changed" in selectors and check_ad_changed(ad_cfg, ad_cfg_orig, ad_file_relative):
should_include = True
elif "changed" in selectors and command == "update" and _price_reduction.is_auto_price_reduction_due(ad_cfg, ad_file_relative):
# Only the "update" command considers pending price reductions
# as a reason to include a "changed" ad.
should_include = True
# Check for 'new' selector
if "new" in selectors and (not ad_cfg.id or not exclude_ads_with_id):
should_include = True
elif "new" in selectors and ad_cfg.id and exclude_ads_with_id:
LOG.info(" -> SKIPPED: ad [%s] is not new. already has an id assigned.", ad_file_relative)
# Check for 'due' selector
if "due" in selectors:
if check_ad_republication(ad_cfg, ad_file_relative):
should_include = True
# Check for 'all' selector (always include)
if "all" in selectors:
should_include = True
if not should_include:
continue
ensure(
get_ad_description(ad_cfg, ad_defaults, with_affixes = False),
_("-> property [description] not specified @ [%s]") % ad_file,
)
get_ad_description(ad_cfg, ad_defaults, with_affixes = True) # validates complete description
resolve_ad_category(ad_cfg, categories)
if ad_cfg.images:
ad_cfg.images = resolve_ad_images(ad_file, ad_cfg.images)
_prepare_selected_ad_entry(ad_file, ad_cfg, ad_defaults, categories)
LOG.info(" -> LOADED: ad [%s]", ad_file_relative)
ads.append((ad_file, ad_cfg, ad_cfg_orig))

8
src/kleinanzeigen_bot/resources/translations.de.yaml
View File

@@ -456,12 +456,16 @@ kleinanzeigen_bot/ad_loading.py:
"Start fetch task for the ad(s) with id(s):": "Starte Abrufaufgabe für die Anzeige(n) mit ID(s):"
" -> SKIPPED: inactive ad [%s]": " -> ÜBERSPRUNGEN: inaktive Anzeige [%s]"
" -> SKIPPED: ad [%s] is not in list of given ids.": " -> ÜBERSPRUNGEN: Anzeige [%s] ist nicht in der Liste der angegebenen IDs."
" -> SKIPPED: ad [%s] is not new. already has an id assigned.": " -> ÜBERSPRUNGEN: Anzeige [%s] ist nicht neu. Eine ID wurde bereits zugewiesen."
"-> property [description] not specified @ [%s]": "-> Eigenschaft [description] nicht angegeben @ [%s]"
" -> LOADED: ad [%s]": " -> GELADEN: Anzeige [%s]"
"Loaded %s": "%s geladen"
"ad": "Anzeige"
_should_include_ad:
" -> SKIPPED: ad [%s] is not new. already has an id assigned.": " -> ÜBERSPRUNGEN: Anzeige [%s] ist nicht neu. Eine ID wurde bereits zugewiesen."
_prepare_selected_ad_entry:
"-> property [description] not specified @ [%s]": "-> Eigenschaft [description] nicht angegeben @ [%s]"
resolve_ad_category:
"Category [%s] unknown. Using category [%s] with ID [%s] instead.": "Kategorie [%s] unbekannt. Verwende stattdessen Kategorie [%s] mit ID [%s]."

38
tests/unit/test_ad_loading.py
View File

@@ -731,6 +731,44 @@ def test_load_ads_skips_inactive_before_numeric_id(
assert len(ads) == 0
def test_skipped_ads_do_not_validate_description_or_images(
base_ad_config:dict[str, Any], test_bot_config:Config,
) -> None:
"""Inactive or non-selected ads are skipped without description/image validation."""
ad_defaults = test_bot_config.with_values(
{"ad_defaults": {"description": {"prefix": "", "suffix": ""}}}
).ad_defaults
inactive_ad = base_ad_config | {
"id": 999,
"title": "Broken Inactive",
"active": False,
"images": ["missing/*.jpg"],
}
# Deliberately include images that would fail resolution (no matches)
# on a selected ad — but the ad is inactive so _prepare_selected_ad_entry never runs
with tempfile.TemporaryDirectory() as temp_dir:
temp_path = Path(temp_dir)
ad_dir = temp_path / "ads"
ad_dir.mkdir()
dicts.save_dict(ad_dir / "inactive_broken.yaml", inactive_ad)
config_file = temp_path / "config.yaml"
config_file.write_text("")
# Should NOT raise — inactive ad is skipped before description/image validation
ads = load_ads(
config_file_path = str(config_file),
ad_file_patterns = ["ads/*.yaml"],
ad_defaults = ad_defaults,
categories = {},
ads_selector = "all",
command = "publish",
)
assert len(ads) == 0
# --------------------------------------------------------------------------- #
# Helpers
# --------------------------------------------------------------------------- #