DEV Community

Cover image for What a tiny Django ticket taught me about open source
kihuni
kihuni

Posted on

What a tiny Django ticket taught me about open source

#django #python #opensource #git

Hi, I'm Stephen, a Djangonaut contributing to Django core. This was my second ticket, and it looked tiny: a one-line capitalization bug in makemessages. It turned into a lesson in code review, minimal diffs, and a lot of git 😂.

Here is the story.

The bug: a helpful message that gives wrong advice

Django's makemessages command extracts translatable strings into .po files, one folder per locale. If you pass a locale that doesn't exist, it tries to help by suggesting a valid one.

Here is what happened when you typed a script-based locale like Simplified Chinese:

$ python manage.py makemessages --locale zh-hans
invalid locale zh-hans, did you mean zh_HAns?
Enter fullscreen mode Exit fullscreen mode

zh-hans is invalid because Django expects an underscore. But the suggestion, zh_HAns, is also invalid. The correct locale is zh_Hans.

The bug was reported on the Django forum and filed as ticket #36642.

Why it happened

Locales come in several shapes:

Locale Parts
de language
pt_BR language + region (2 letters)
en_001 language + numeric region
zh_Hans language + script (4 letters)
nl_NL-x-informal language + region + private use

The suggestion code split the input into a language and "everything after the separator", then assumed that part was a country code and uppercased its first two characters:

territory = (
    locale_parts["territory"][:2].upper()
    + locale_parts["territory"][2:]
)
Enter fullscreen mode Exit fullscreen mode

That is right for pl_pl → pl_PL. For hans it gives HA + ns, which is HAns. The code was written for one locale shape and silently applied to all of them.

One constraint shaped everything: any fix had to leave validation alone. en_001 was valid before, and a fix that broke it would trade one bug for another.

Test first

Django's workflow is to write the test, watch it fail, then fix. My regression test copied the structure of the neighbouring test_invalid_locale_lower_country:

def test_invalid_locale_mixed_case_script(self):
    out = StringIO()
    management.call_command(
        "makemessages", locale=["zh-hans"], stdout=out, verbosity=1
    )
    self.assertIn("invalid locale zh-hans, did you mean zh_Hans?", out.getvalue())
    self.assertNotIn("processing locale zh-hans", out.getvalue())
    self.assertIs(Path("locale/zh-hans/LC_MESSAGES/django.po").exists(), False)
Enter fullscreen mode Exit fullscreen mode

Before the fix, it failed exactly as the ticket described:

AssertionError: 'invalid locale zh-hans, did you mean zh_Hans?' not found in
'invalid locale zh-hans, did you mean zh_HAns?\n'
Enter fullscreen mode Exit fullscreen mode

[Add your failing-test screenshot here]

The fix, three versions

Version 1: the first PR

My first PR looked locales up in Django's LANG_INFO dictionary. It fixed zh-hans, but en_001 stopped being recognized as valid. The reviewer, Jacob Walls, caught that regression. I left is_valid_locale() alone and changed only how the suggestion is formatted.

Version 2: format each subtag by type 

def format_subtag(m):
    subtag = m.group()
    if len(subtag) == 4:  # script, e.g. Hans
        return subtag.title()
    if len(subtag) == 2:  # region, e.g. CN
        return subtag.upper()
    return subtag

territory = re.sub(r"[a-zA-Z]+", format_subtag, territory) + x + private
Enter fullscreen mode Exit fullscreen mode

Private-use parts like -x-informal were split off first with partition("-x-") so they stayed untouched.

Review: "Why special-case length 4?"

Jacob pointed out that another Chinese locale, zh-yue, has a three-letter subtag. A rule built on "scripts are 4 letters" was a guess. Only the 2-letter case needs upper(), so the helper and re.sub could go.

Version 3: the simple rule

Jacob suggested a simpler approach that handles only the 2-letter case. I made the change:

# Two-letter subtags are region codes (e.g. GB), which
# are uppercased. Others, such as scripts (e.g. Hans),
# are title-cased.
if len(territory) == 2:
    territory = territory.upper()
else:
    territory = territory.title()
proposed_locale = f"{language}_{territory}" + x + private
Enter fullscreen mode Exit fullscreen mode
Input Suggestion
zh-hans zh_Hans
pl_pl pl_PL
en-001 en_001
nl-nl-x-informal nl_NL-x-informal

He also suggested .title() over slicing, and asked for a comment explaining why 2-letter strings are special. Both made the code easier for the next reader.

What I chose not to solve

zh_hans_cn still suggests zh_Hans_cn, which is invalid. Fixing it means deciding which subtag is a script, a region, or a variant, and without a length rule, that is guessing. Jacob said that coming back with "I explored it, but it's too error-prone" would also move the PR forward. Knowing when to stop was part of the fix.

The messy middle

The code took an afternoon. The git around it took longer 😂.

  • Old base: my branch was built on old code, so the diff against main listed hundreds of files. I backed it up with git branch backup, ran git reset --hard upstream/main, and re-applied only my changes by hand.
  • A file I overwrote: test_extraction.py had been replaced with an older copy, which reverted unrelated tests and triggered flake8 errors on lines I never touched. git diff revealed it.
  • Push rejected as "stale info": --force-with-lease had no record of the remote branch. git ls-remote --heads origin showed what was actually there.
  • Wrong remote: I once pointed upstream at my own fork instead of django/django.
  • A push that never landed: GitHub looked unchanged. Comparing git log with git log origin/<branch> showed the push had not gone through.
  • The PR bot: it failed my PR on an "Incomplete Checklist" because the boxes had lost their - [x] syntax when I copied the description.

Lessons for first-time contributors

  1. Keep diffs minimal. Reviewers notice whitespace and cosmetic changes.
  2. Verify, don't assume. Check git diff --stat, git log and the remote after every big step.
  3. Back up before destructive commands.
  4. Specific test names beat docstrings.
  5. Reviewers can give you a way out.
  6. Expect the workflow to take longer than the code.
  7. A small ticket is the best way to learn the whole process.

Thank you to Jacob Walls and Tim Schilling for the patient reviews.

Links

Top comments (1)

Collapse
 
lycore profile image
Lycore Development •

Separating suggestion formatting from validation is the key design lesson here: a helpful error message should not quietly redefine which locales are accepted. I would keep the numeric-region and private-use examples next to the regression test as guardrails for future simplifications. Did the review also add a test that en_001 remains accepted, or is that already covered by the neighbouring tests?