Skip to content

Conversation

@iShelar
Copy link
Contributor

@iShelar iShelar commented Jan 23, 2026

  • Introduced a new development.env file for environment variable configuration.
  • Updated README to reflect new task command for generating sample data.
  • Added django:generate-sample-data task to Taskfile for easier sample data generation.
  • Refactored factories in factories.py to use Wagtail factories and improved sample data generation logic in the management command.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you modify requirements/dev.in (for example by adding wagtail-factories), you need to remember to regenerate the lockfile requirements/dev.txt. To do this, you can simply run task dependencies:compute. This is important because it will resolve all transitive dependencies and ensure everyone uses the same versions.

For the commit message, you can explain that you've recompiled the dependencies and remind that after any modification to requirements/dev.in, this command needs to be run again. It's helpful for other contributors who might not be familiar with this workflow.

class Meta:
model = SponsorshipLevel
django_get_or_create = ("name",)
def create_sponsorship_level(name="Bronze", level=100):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that in commit 33e042e, you replaced the existing factory-boy classes with wrapper functions. I'd like to explain why the original approach was actually better and should have been kept.

What Changed

Before your commit, we had proper factory-boy classes:

class SponsorshipLevelFactory(DjangoModelFactory):                                                                                  
    class Meta:                                                                                                                     
        model = SponsorshipLevel                                                                                                    
        django_get_or_create = ("name",)                                                                                            
                                                                                                                                    
    name = "Bronze"                                                                                                                 
    level = 100 

After your commit, these became simple wrapper functions:

def SponsorshipLevelFactory(name="Bronze", level=100):                                                                              
    return create_sponsorship_level(name=name, level=level)                                                                         

Why This Is a Step Backward

The original code was following factory-boy best practices. Here's why the wrapper functions are problematic:

  1. Naming Convention Violation: In Python (PEP 8), functions should use snake_case, not PascalCase. When developers see
    SponsorshipLevelFactory, they expect a class, not a function. This creates confusion.
  2. Lost Framework Features: The wrapper functions don't use any of factory-boy's powerful features like factory.Sequence(),
    factory.LazyFunction(), factory.SubFactory(), etc. You're essentially bypassing the framework you've added as a dependency.
  3. Misleading API: The name suggests it's a factory-boy factory, but it's just a simple wrapper around Django's get_or_create. This
    is misleading for anyone reading or maintaining the code.
  4. Unnecessary Abstraction Layer: You've added an extra layer (SponsorshipLevelFactory → create_sponsorship_level →
    SponsorshipLevel.objects.get_or_create) that doesn't add value. The factory-boy class was already the right abstraction.

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.

3 participants