Skip to content

Closes #22438: Rename jinja2_filters/get_jinja2_context/register_jinja2_filters to drop the '2' suffix#22437

Open
bctiemann wants to merge 1 commit into
featurefrom
22436-rename-jinja2-identifiers
Open

Closes #22438: Rename jinja2_filters/get_jinja2_context/register_jinja2_filters to drop the '2' suffix#22437
bctiemann wants to merge 1 commit into
featurefrom
22436-rename-jinja2-identifiers

Conversation

@bctiemann

@bctiemann bctiemann commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #22438. Follow-up housekeeping to #22363.

PR #22363 introduced several plugin hook identifiers with a jinja2 prefix. The JINJA2_FILTERS setting was renamed to JINJA_FILTERS in #22288 before that PR merged, but the hook names were not updated to match. This PR aligns them:

  • PluginConfig.jinja2_filtersjinja_filters
  • PluginConfig.get_jinja2_context()get_jinja_context()
  • register_jinja2_filters()register_jinja_filters()
  • Default auto-discovery file jinja2_env.pyjinja_env.py

All call sites, tests, and documentation updated accordingly. No functional changes.

Test plan

  • Existing plugin tests (netbox.tests.test_plugins) pass unchanged (only renamed)
  • Verify dummy plugin's jinja_env.py is auto-discovered and its filter appears in registry['plugins']['jinja_filters']

🤖 Generated with Claude Code

…a2_filters to drop the '2' suffix

Follow-up to #22363: align the plugin hook names with the already-renamed
JINJA_FILTERS setting (#22288) and with the rest of the codebase's 'Jinja'
spelling convention.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bctiemann bctiemann changed the title Closes #22436: Rename jinja2_filters/get_jinja2_context/register_jinja2_filters to drop the '2' suffix Closes #22438: Rename jinja2_filters/get_jinja2_context/register_jinja2_filters to drop the '2' suffix Jun 10, 2026
@bctiemann

Copy link
Copy Markdown
Contributor Author

@claude /review and limit response to areas of potential concern.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @bctiemann's task in 2m 15s —— View job


PR Review

The rename is mechanically complete — no old jinja2_* identifiers remain in the codebase. The diff is clean, symmetric, and consistent across all layers (registry, PluginConfig, docs, tests, dummy plugin). Below are the areas worth scrutiny.


1. Breaking API change with no deprecation path

Since these identifiers were introduced in PR #22363 (not yet released, targeting the feature branch), the pool of affected plugin authors is small. Still, any plugin that already adopted these APIs will break in three distinct ways with differing failure modes:

Old usage New behavior
jinja2_filters attribute on PluginConfig _load_resource('jinja_filters') does getattr(self, 'jinja_filters', None) — silently falls through to jinja_env.filters autodiscovery instead
get_jinja2_context() override Base class get_jinja_context() returns {}silent failure, context variables simply disappear
from netbox.plugins.registration import register_jinja2_filters Raises ImportError — at least this one is loud
jinja2_env.py auto-discovery file Module not found, filters silently not registered

The silent get_jinja2_context() case (extras/models/mixins.py:144) is particularly hard to debug: a plugin subclass overriding the old name will still be imported and loaded without error, but its injected context will simply be empty. No warning, no log message. Consider whether a one-line warnings.warn() shim on the old method name would be worthwhile for the initial release.

2. _load_resource KeyError on unrecognized names

plugins/__init__.py:110 accesses DEFAULT_RESOURCE_PATHS[name] with a bare [] lookup (no .get()). This is an existing pattern, not introduced here. But it's worth noting that if any future call path supplies an unrecognized resource name, it will raise KeyError rather than returning None. Not a bug this PR introduces, but the rename makes it marginally more likely to bite early adopters testing with a partially-updated codebase.

3. Test cleanup relies on direct registry mutation

test_register_jinja_filters_merges_into_registry (test_plugins.py:311–318) inserts and then dels a key from registry['plugins']['jinja_filters']. This works, but if the test crashes before the finally, the registry is left dirty for subsequent tests in the same run. The test does use try/finally correctly, so this is low-risk — just worth verifying the test isolation if the suite is ever run with --keepdb.

4. No concern found in the core rename

The utilities/jinja2.py:105 change to registry['plugins'].get('jinja_filters', {}) (with a {} fallback) is safe — the registry is always initialized with the key present, so the fallback is unreachable but harmless.

All nine changed files are consistent; no leftover jinja2_* references were found in the working tree.

@bctiemann bctiemann requested review from a team and jeremystretch and removed request for a team June 10, 2026 22:53
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.

1 participant