Add category and section populators; enhance dependency handling#1241
Open
Add category and section populators; enhance dependency handling#1241
Conversation
Contributor
WalkthroughThis pull request enhances the test data generation system across multiple management command files. It adds validation and bounds checking to the dependency linking logic. Two new content populators are introduced: CategoryPopulator creates predefined package categories per community, and SectionPopulator defines and creates five package listing sections with specific category configurations. Package listings are extended to receive random category assignments. The new populators are registered in the main test data creation command to integrate them into the population workflow. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1241 +/- ##
========================================
Coverage 93.03% 93.04%
========================================
Files 344 346 +2
Lines 10643 10755 +112
Branches 954 968 +14
========================================
+ Hits 9902 10007 +105
- Misses 617 622 +5
- Partials 124 126 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
django/thunderstore/core/management/commands/content/package_listing.py (1)
18-43:⚠️ Potential issue | 🟠 MajorUse
try/finallywhen disconnecting model signalsIf an exception happens during population (e.g., Line 39), the reconnect block is skipped and signals stay disconnected for the process lifetime.
Suggested fix
- signals.post_save.disconnect(PackageListing.post_save, sender=PackageListing) - signals.post_delete.disconnect( - PackageListing.post_delete, sender=PackageListing - ) - - community_categories = {} - for community in context.communities: - community_categories[community.id] = list( - PackageCategory.objects.filter(community=community) - ) - - for i, package in print_progress( - enumerate(context.packages), len(context.packages) - ): - for community in context.communities: - listing = package.get_or_create_package_listing(community) - - cats = community_categories.get(community.id, []) - if cats: - count = random.choices([1, 2, 3, 4, 5], weights=[40, 30, 15, 10, 5])[0] - chosen = random.sample(cats, min(count, len(cats))) - listing.categories.set(chosen) - - # Re-enabling previously disabled signals - signals.post_save.connect(PackageListing.post_save, sender=PackageListing) - signals.post_delete.connect(PackageListing.post_delete, sender=PackageListing) + signals.post_save.disconnect(PackageListing.post_save, sender=PackageListing) + signals.post_delete.disconnect(PackageListing.post_delete, sender=PackageListing) + try: + community_categories = {} + for community in context.communities: + community_categories[community.id] = list( + PackageCategory.objects.filter(community=community) + ) + + for _i, package in print_progress( + enumerate(context.packages), len(context.packages) + ): + for community in context.communities: + listing = package.get_or_create_package_listing(community) + cats = community_categories.get(community.id, []) + if cats: + count = random.choices([1, 2, 3, 4, 5], weights=[40, 30, 15, 10, 5])[0] + chosen = random.sample(cats, min(count, len(cats))) + listing.categories.set(chosen) + else: + listing.categories.clear() + finally: + # Re-enabling previously disabled signals + signals.post_save.connect(PackageListing.post_save, sender=PackageListing) + signals.post_delete.connect(PackageListing.post_delete, sender=PackageListing)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django/thunderstore/core/management/commands/content/package_listing.py` around lines 18 - 43, Disconnecting model signals (signals.post_save.disconnect/ signals.post_delete.disconnect for PackageListing.post_save and PackageListing.post_delete) is not protected, so if an exception occurs while populating listings (e.g., in the loop over context.packages / package.get_or_create_package_listing), signals will remain disconnected; wrap the disconnect/connect sequence in a try/finally: perform the two disconnect calls before the population work and ensure the two connect calls (signals.post_save.connect and signals.post_delete.connect for PackageListing) are executed in the finally block so signals are always re-enabled even on error.
🧹 Nitpick comments (2)
django/thunderstore/core/management/commands/content/package_listing.py (1)
29-31: Optional lint cleanup: unused loop variableLine 29 introduces
ibut it is never used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django/thunderstore/core/management/commands/content/package_listing.py` around lines 29 - 31, The for-loop is enumerating context.packages into (i, package) but never uses i; update the loop in package_listing (the for using print_progress and enumerate over context.packages) to drop the unused index by either iterating directly (for package in print_progress(context.packages, len(context.packages))) or use a throwaway variable (_) instead of i so the unused variable warning is eliminated.django/thunderstore/core/management/commands/content/package_categories.py (1)
12-43: Consider makingcategoriesimmutableThis class-level list is effectively constant; switching to a tuple avoids accidental mutation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django/thunderstore/core/management/commands/content/package_categories.py` around lines 12 - 43, The class-level variable categories is currently a mutable list; change it to an immutable tuple (replace the [] with ()) so it becomes a constant, and if any code in this module references or mutates categories (e.g., appending or extending), update those sites to operate on a copied list (list(categories)) or use a separate mutable variable instead; ensure the symbol categories is updated everywhere in this module to the tuple form to prevent accidental mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@django/thunderstore/core/management/commands/content/package_categories.py`:
- Around line 1-58: This file has mixed line endings causing pre-commit to
modify it; normalize the line endings to LF for the module containing class
CategoryPopulator (and its methods populate and clear) and the categories list
so pre-commit no longer mutates the file—open the file in your editor or run a
tool like dos2unix/gh or set git's eol handling (e.g., git add --renormalize .
or set core.autocrlf appropriately) to convert CRLF to LF, commit the normalized
file, and re-run the checks.
- Line 1: Remove the unused typing import by deleting the "from typing import
List" statement at the top of the module; the symbol List is not referenced
anywhere in this file (package_categories.py), so simply remove that import line
to satisfy the linter.
In `@django/thunderstore/core/management/commands/content/package_listing.py`:
- Around line 35-39: The current block uses community_categories and skips
assignment when cats is empty, which leaves previous relations intact; update
the logic around the listing.categories assignment (the code that computes cats,
count, chosen and calls listing.categories.set) to explicitly clear categories
when cats is empty (e.g., call listing.categories.clear() or set an empty list)
instead of skipping assignment so stale categories are removed on reruns.
In `@django/thunderstore/core/management/commands/content/package_sections.py`:
- Around line 1-84: The file has mixed line endings causing pre-commit
normalization failures; open the file containing the SectionPopulator class
(methods populate and clear, references to PackageListingSection and
PackageCategory), convert all CRLF to LF (Unix) line endings, save and re-commit
the file so git records the normalized endings; optionally run a tool like
dos2unix or set core.autocrlf/.gitattributes and re-stage the file before
committing to ensure pre-commit no longer modifies it.
- Around line 57-80: The current logic only handles creations because the update
code is inside the "if created" branch, so existing PackageListingSection rows
never get their name, priority, or exclude/require category filters updated;
change the code to always set section.name and section.priority and save the
section (regardless of created), and then synchronise the M2M relations for
exclude_categories and require_categories by resolving s_data["exclude"] and
s_data["require"] via get_cat into a list (skipping/logging
PackageCategory.DoesNotExist) and then replace the relations (e.g., clear() then
add() or set([...])) instead of only adding when created so re-runs update
existing PackageListingSection records correctly.
---
Outside diff comments:
In `@django/thunderstore/core/management/commands/content/package_listing.py`:
- Around line 18-43: Disconnecting model signals (signals.post_save.disconnect/
signals.post_delete.disconnect for PackageListing.post_save and
PackageListing.post_delete) is not protected, so if an exception occurs while
populating listings (e.g., in the loop over context.packages /
package.get_or_create_package_listing), signals will remain disconnected; wrap
the disconnect/connect sequence in a try/finally: perform the two disconnect
calls before the population work and ensure the two connect calls
(signals.post_save.connect and signals.post_delete.connect for PackageListing)
are executed in the finally block so signals are always re-enabled even on
error.
---
Nitpick comments:
In `@django/thunderstore/core/management/commands/content/package_categories.py`:
- Around line 12-43: The class-level variable categories is currently a mutable
list; change it to an immutable tuple (replace the [] with ()) so it becomes a
constant, and if any code in this module references or mutates categories (e.g.,
appending or extending), update those sites to operate on a copied list
(list(categories)) or use a separate mutable variable instead; ensure the symbol
categories is updated everywhere in this module to the tuple form to prevent
accidental mutation.
In `@django/thunderstore/core/management/commands/content/package_listing.py`:
- Around line 29-31: The for-loop is enumerating context.packages into (i,
package) but never uses i; update the loop in package_listing (the for using
print_progress and enumerate over context.packages) to drop the unused index by
either iterating directly (for package in print_progress(context.packages,
len(context.packages))) or use a throwaway variable (_) instead of i so the
unused variable warning is eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d675c9a1-652b-4cfd-b79d-529e6863f309
📒 Files selected for processing (5)
django/thunderstore/core/management/commands/content/dependencies.pydjango/thunderstore/core/management/commands/content/package_categories.pydjango/thunderstore/core/management/commands/content/package_listing.pydjango/thunderstore/core/management/commands/content/package_sections.pydjango/thunderstore/core/management/commands/create_test_data.py
Comment on lines
+1
to
+58
| from typing import List | ||
|
|
||
| from thunderstore.community.models import PackageCategory | ||
| from thunderstore.core.management.commands.content.base import ( | ||
| ContentPopulator, | ||
| ContentPopulatorContext, | ||
| ) | ||
| from thunderstore.utils.iterators import print_progress | ||
|
|
||
|
|
||
| class CategoryPopulator(ContentPopulator): | ||
| categories = [ | ||
| "Mods", | ||
| "Modpacks", | ||
| "Tools", | ||
| "Libraries", | ||
| "Misc", | ||
| "Audio", | ||
| "BepInEx", | ||
| "MelonLoader", | ||
| "Suits", | ||
| "Boombox Music", | ||
| "TV Videos", | ||
| "Posters", | ||
| "Equipment", | ||
| "Items", | ||
| "Monsters", | ||
| "Moons", | ||
| "Interiors", | ||
| "Furniture", | ||
| "Vehicles", | ||
| "Client-side", | ||
| "Server-side", | ||
| "Cosmetics", | ||
| "Asset Replacements", | ||
| "Translations", | ||
| "Emotes", | ||
| "Weather", | ||
| "Hazards", | ||
| "Bug Fixes", | ||
| "Performance", | ||
| "Tweaks & Quality Of Life", | ||
| ] | ||
|
|
||
| def populate(self, context: ContentPopulatorContext) -> None: | ||
| print("Populating package categories...") | ||
| for community in print_progress(context.communities, len(context.communities)): | ||
| for name in self.categories: | ||
| slug = name.lower().replace(" ", "-").replace("&", "").replace("--", "-") | ||
| PackageCategory.objects.get_or_create( | ||
| community=community, | ||
| name=name, | ||
| defaults={"slug": slug}, | ||
| ) | ||
|
|
||
| def clear(self) -> None: | ||
| print("Deleting existing package categories...") | ||
| PackageCategory.objects.all().delete() |
Contributor
There was a problem hiding this comment.
Normalize line endings to pass pre-commit
CI indicates mixed line endings in this file, and pre-commit is mutating it.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 1-1: 'typing.List' imported but unused
(F401)
🪛 GitHub Actions: Build & Test
[error] 1-1: Mixed line endings detected and file was modified by pre-commit hook (mixed-line-ending).
🪛 Ruff (0.15.6)
[warning] 12-43: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django/thunderstore/core/management/commands/content/package_categories.py`
around lines 1 - 58, This file has mixed line endings causing pre-commit to
modify it; normalize the line endings to LF for the module containing class
CategoryPopulator (and its methods populate and clear) and the categories list
so pre-commit no longer mutates the file—open the file in your editor or run a
tool like dos2unix/gh or set git's eol handling (e.g., git add --renormalize .
or set core.autocrlf appropriately) to convert CRLF to LF, commit the normalized
file, and re-run the checks.
| @@ -0,0 +1,58 @@ | |||
| from typing import List | |||
Contributor
There was a problem hiding this comment.
Remove unused import List
Line 1 imports List but nothing in this file uses it (F401).
Suggested fix
-from typing import List
-
from thunderstore.community.models import PackageCategory📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from typing import List | |
| from thunderstore.community.models import PackageCategory |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 1-1: 'typing.List' imported but unused
(F401)
🪛 GitHub Actions: Build & Test
[error] 1-1: Mixed line endings detected and file was modified by pre-commit hook (mixed-line-ending).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django/thunderstore/core/management/commands/content/package_categories.py`
at line 1, Remove the unused typing import by deleting the "from typing import
List" statement at the top of the module; the symbol List is not referenced
anywhere in this file (package_categories.py), so simply remove that import line
to satisfy the linter.
Comment on lines
+35
to
+39
| cats = community_categories.get(community.id, []) | ||
| if cats: | ||
| count = random.choices([1, 2, 3, 4, 5], weights=[40, 30, 15, 10, 5])[0] | ||
| chosen = random.sample(cats, min(count, len(cats))) | ||
| listing.categories.set(chosen) |
Contributor
There was a problem hiding this comment.
Clear stale categories when a community has no available categories
On reruns, Line 36 skips assignment when cats is empty, so previous category relations can persist unexpectedly.
🧰 Tools
🪛 Ruff (0.15.6)
[error] 37-37: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django/thunderstore/core/management/commands/content/package_listing.py`
around lines 35 - 39, The current block uses community_categories and skips
assignment when cats is empty, which leaves previous relations intact; update
the logic around the listing.categories assignment (the code that computes cats,
count, chosen and calls listing.categories.set) to explicitly clear categories
when cats is empty (e.g., call listing.categories.clear() or set an empty list)
instead of skipping assignment so stale categories are removed on reruns.
Comment on lines
+1
to
+84
| from thunderstore.community.models import PackageCategory, PackageListingSection | ||
| from thunderstore.core.management.commands.content.base import ( | ||
| ContentPopulator, | ||
| ContentPopulatorContext, | ||
| ) | ||
| from thunderstore.utils.iterators import print_progress | ||
|
|
||
|
|
||
| class SectionPopulator(ContentPopulator): | ||
| def populate(self, context: ContentPopulatorContext) -> None: | ||
| print("Populating package listing sections...") | ||
| for community in print_progress(context.communities, len(context.communities)): | ||
|
|
||
| def get_cat(slug): | ||
| return PackageCategory.objects.get(community=community, slug=slug) | ||
|
|
||
| # Define sections | ||
| # Note: Higher priority value means the section is shown first (descending sort). | ||
| sections_data = [ | ||
| { | ||
| "name": "Mods", | ||
| "slug": "mods", | ||
| "priority": 100, | ||
| "exclude": ["modpacks", "asset-replacements"], | ||
| "require": [], | ||
| }, | ||
| { | ||
| "name": "Asset Replacements", | ||
| "slug": "asset-replacements", | ||
| "priority": 90, | ||
| "exclude": ["modpacks"], | ||
| "require": ["asset-replacements"], | ||
| }, | ||
| { | ||
| "name": "Tools", | ||
| "slug": "tools", | ||
| "priority": 85, | ||
| "exclude": ["modpacks"], | ||
| "require": ["tools"], | ||
| }, | ||
| { | ||
| "name": "APIs & Libraries", | ||
| "slug": "libraries", | ||
| "priority": 80, | ||
| "exclude": ["modpacks"], | ||
| "require": ["libraries"], | ||
| }, | ||
| { | ||
| "name": "Modpacks", | ||
| "slug": "modpacks", | ||
| "priority": 70, | ||
| "exclude": [], | ||
| "require": ["modpacks"], | ||
| }, | ||
| ] | ||
|
|
||
| for s_data in sections_data: | ||
| section, created = PackageListingSection.objects.get_or_create( | ||
| community=community, | ||
| slug=s_data["slug"], | ||
| defaults={ | ||
| "name": s_data["name"], | ||
| "priority": s_data["priority"], | ||
| }, | ||
| ) | ||
|
|
||
| if created: | ||
| for ex_slug in s_data["exclude"]: | ||
| try: | ||
| cat = get_cat(ex_slug) | ||
| section.exclude_categories.add(cat) | ||
| except PackageCategory.DoesNotExist: | ||
| print(f"Warning: Category {ex_slug} not found for section {s_data['slug']}") | ||
|
|
||
| for req_slug in s_data["require"]: | ||
| try: | ||
| cat = get_cat(req_slug) | ||
| section.require_categories.add(cat) | ||
| except PackageCategory.DoesNotExist: | ||
| print(f"Warning: Category {req_slug} not found for section {s_data['slug']}") | ||
|
|
||
| def clear(self) -> None: | ||
| print("Deleting existing package listing sections...") | ||
| PackageListingSection.objects.all().delete() |
Contributor
There was a problem hiding this comment.
Normalize line endings in this file
Pipeline shows mixed line endings and pre-commit auto-modifies this file.
🧰 Tools
🪛 GitHub Actions: Build & Test
[error] 1-1: Mixed line endings detected and file was modified by pre-commit hook (mixed-line-ending).
🪛 Ruff (0.15.6)
[warning] 15-15: Function definition does not bind loop variable community
(B023)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django/thunderstore/core/management/commands/content/package_sections.py`
around lines 1 - 84, The file has mixed line endings causing pre-commit
normalization failures; open the file containing the SectionPopulator class
(methods populate and clear, references to PackageListingSection and
PackageCategory), convert all CRLF to LF (Unix) line endings, save and re-commit
the file so git records the normalized endings; optionally run a tool like
dos2unix or set core.autocrlf/.gitattributes and re-stage the file before
committing to ensure pre-commit no longer modifies it.
Comment on lines
+57
to
+80
| for s_data in sections_data: | ||
| section, created = PackageListingSection.objects.get_or_create( | ||
| community=community, | ||
| slug=s_data["slug"], | ||
| defaults={ | ||
| "name": s_data["name"], | ||
| "priority": s_data["priority"], | ||
| }, | ||
| ) | ||
|
|
||
| if created: | ||
| for ex_slug in s_data["exclude"]: | ||
| try: | ||
| cat = get_cat(ex_slug) | ||
| section.exclude_categories.add(cat) | ||
| except PackageCategory.DoesNotExist: | ||
| print(f"Warning: Category {ex_slug} not found for section {s_data['slug']}") | ||
|
|
||
| for req_slug in s_data["require"]: | ||
| try: | ||
| cat = get_cat(req_slug) | ||
| section.require_categories.add(cat) | ||
| except PackageCategory.DoesNotExist: | ||
| print(f"Warning: Category {req_slug} not found for section {s_data['slug']}") |
Contributor
There was a problem hiding this comment.
Section definitions are not synced for existing rows
Because updates are gated behind if created (Line 67), reruns won’t update name, priority, or category filters for existing sections.
Suggested fix
- section, created = PackageListingSection.objects.get_or_create(
+ section, _created = PackageListingSection.objects.get_or_create(
community=community,
slug=s_data["slug"],
defaults={
"name": s_data["name"],
"priority": s_data["priority"],
},
)
-
- if created:
- for ex_slug in s_data["exclude"]:
- try:
- cat = get_cat(ex_slug)
- section.exclude_categories.add(cat)
- except PackageCategory.DoesNotExist:
- print(f"Warning: Category {ex_slug} not found for section {s_data['slug']}")
-
- for req_slug in s_data["require"]:
- try:
- cat = get_cat(req_slug)
- section.require_categories.add(cat)
- except PackageCategory.DoesNotExist:
- print(f"Warning: Category {req_slug} not found for section {s_data['slug']}")
+ section.name = s_data["name"]
+ section.priority = s_data["priority"]
+ section.save(update_fields=["name", "priority"])
+
+ exclude_cats = []
+ for ex_slug in s_data["exclude"]:
+ try:
+ exclude_cats.append(get_cat(ex_slug))
+ except PackageCategory.DoesNotExist:
+ print(f"Warning: Category {ex_slug} not found for section {s_data['slug']}")
+
+ require_cats = []
+ for req_slug in s_data["require"]:
+ try:
+ require_cats.append(get_cat(req_slug))
+ except PackageCategory.DoesNotExist:
+ print(f"Warning: Category {req_slug} not found for section {s_data['slug']}")
+
+ section.exclude_categories.set(exclude_cats)
+ section.require_categories.set(require_cats)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django/thunderstore/core/management/commands/content/package_sections.py`
around lines 57 - 80, The current logic only handles creations because the
update code is inside the "if created" branch, so existing PackageListingSection
rows never get their name, priority, or exclude/require category filters
updated; change the code to always set section.name and section.priority and
save the section (regardless of created), and then synchronise the M2M relations
for exclude_categories and require_categories by resolving s_data["exclude"] and
s_data["require"] via get_cat into a list (skipping/logging
PackageCategory.DoesNotExist) and then replace the relations (e.g., clear() then
add() or set([...])) instead of only adding when created so re-runs update
existing PackageListingSection records correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.