quality/python-wikitcms

Fork 0

Tweak FMW env special handling to return real column names #15

Merged

adamwill merged 1 commit from fixup-fmw-handling into main

2026-04-10 00:50:55 +00:00

adamwill commented

2026-04-10 00:43:42 +00:00

Owner

The placeholder names aren't good enough - we have to substitute
with the real names, as best we can (we'll use fedfind.helpers
for this, which should pretty much always be in sync with
the CurrentFedoraVersion template; we can't easily use the
template here because Past Adam foolishly didn't put the wiki
object in this function's scope).

This substitution is actually rarely used anyway; it'll only be
used if you instantiate Template:Installation test matrix rather
than an actual result page, and the only thing that does that is
the fedora_openqa checkwiki script (for...reasons). Added a
note explaining this.

Signed-off-by: Adam Williamson awilliam@redhat.com

The placeholder names aren't good enough - we have to substitute with the *real* names, as best we can (we'll use fedfind.helpers for this, which should pretty much always be in sync with the CurrentFedoraVersion template; we can't easily use the template here because Past Adam foolishly didn't put the wiki object in this function's scope). This substitution is actually rarely used anyway; it'll only be used if you instantiate Template:Installation test matrix rather than an actual result page, and the only thing that does that is the fedora_openqa `checkwiki` script (for...reasons). Added a note explaining this. Signed-off-by: Adam Williamson <awilliam@redhat.com>

adamwill added 1 commit

2026-04-10 00:43:42 +00:00

Tweak FMW env special handling to return real column names

CI via Tox / tox (pull_request) Successful in 1m36s

Details

AI Code Review / ai-review (pull_request_target) Successful in 22s

Details

e168f30282

The placeholder names aren't good enough - we have to substitute
with the *real* names, as best we can (we'll use fedfind.helpers
for this, which should pretty much always be in sync with
the CurrentFedoraVersion template; we can't easily use the
template here because Past Adam foolishly didn't put the wiki
object in this function's scope).

This substitution is actually rarely used anyway; it'll only be
used if you instantiate Template:Installation test matrix rather
than an actual result page, and the only thing that does that is
the fedora_openqa `checkwiki` script (for...reasons). Added a
note explaining this.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

forgejo-actions commented

2026-04-10 00:45:45 +00:00

AI Code Review

📋 MR Summary

This MR updates the Fedora Media Writer environment special handling to return actual column names (e.g., "Fedora 43") rather than placeholders.

Key Changes:
- Added an import for fedfind.helpers in src/wikitcms/result.py.
- Updated logic in find_resultrows to dynamically fetch the current release and compute related release names.
- Mocked fedfind.helpers.get_current_release in tests/test_result.py to test the new column name resolution logic.
Impact: src/wikitcms/result.py, tests/test_result.py
Risk Level: 🟢 Low - The changes are straightforward, limited to edge-case handling for a specific set of wiki columns, and tested appropriately.

Detailed Code Review

The implementation correctly updates the placeholder handling to dynamically calculate Fedora release version names. The code properly manages the calculation of offsets (- 1 and + 1) relative to the current version. The newly introduced functionality is adequately covered by updated unit tests using the mock library. One small observation: depending on the type returned by fedfind.helpers.get_current_release(), calling .split() on the resulting string could potentially trigger an unhandled exception if curr is None or an unexpected type, although the subsequent split("|") logic appears well guarded in its surrounding try...except context. Overall, it's a solid, low-risk improvement.

📂 File Reviews

📄 `src/wikitcms/result.py` - Modified the special handling for media writer column headers to use actual release numbers based on `fedfind.helpers.get_current_release()`.

Minor [Logic]: If fedfind.helpers.get_current_release() returns None (for example, due to an API failure or unexpected state), curr - 1 or curr + 1 will raise a TypeError.
- Suggestion: Add a fallback or check to ensure curr is an integer before attempting arithmetic operations.

curr = fedfind.helpers.get_current_release()
if curr is not None:
    if "- 1" in newcol:
        newcol = f"Fedora {str(curr - 1)}"
    elif "+ 1" in newcol:
        newcol = f"Fedora {str(curr + 1)}"
    else:
        newcol = f"Fedora {str(curr)}"

✅ Summary

Overall Assessment: The changes look good and accomplish the intended goal, with only a minor edge case around null-safety identified.
Minor Suggestions:
- Add a guard check or exception handler around the return value of fedfind.helpers.get_current_release() to protect against potential TypeError.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This MR updates the Fedora Media Writer environment special handling to return actual column names (e.g., "Fedora 43") rather than placeholders. - **Key Changes:** - Added an import for `fedfind.helpers` in `src/wikitcms/result.py`. - Updated logic in `find_resultrows` to dynamically fetch the current release and compute related release names. - Mocked `fedfind.helpers.get_current_release` in `tests/test_result.py` to test the new column name resolution logic. - **Impact:** src/wikitcms/result.py, tests/test_result.py - **Risk Level:** 🟢 Low - The changes are straightforward, limited to edge-case handling for a specific set of wiki columns, and tested appropriately. ### Detailed Code Review The implementation correctly updates the placeholder handling to dynamically calculate Fedora release version names. The code properly manages the calculation of offsets (`- 1` and `+ 1`) relative to the current version. The newly introduced functionality is adequately covered by updated unit tests using the `mock` library. One small observation: depending on the type returned by `fedfind.helpers.get_current_release()`, calling `.split()` on the resulting string could potentially trigger an unhandled exception if `curr` is None or an unexpected type, although the subsequent `split("|")` logic appears well guarded in its surrounding `try...except` context. Overall, it's a solid, low-risk improvement. #### 📂 File Reviews <details> <summary><strong>📄 `src/wikitcms/result.py`</strong> - Modified the special handling for media writer column headers to use actual release numbers based on `fedfind.helpers.get_current_release()`.</summary> - **Minor** [Logic]: If `fedfind.helpers.get_current_release()` returns `None` (for example, due to an API failure or unexpected state), `curr - 1` or `curr + 1` will raise a `TypeError`. - *Suggestion:* Add a fallback or check to ensure `curr` is an integer before attempting arithmetic operations. ```` curr = fedfind.helpers.get_current_release() if curr is not None: if "- 1" in newcol: newcol = f"Fedora {str(curr - 1)}" elif "+ 1" in newcol: newcol = f"Fedora {str(curr + 1)}" else: newcol = f"Fedora {str(curr)}" ```` </details> ### ✅ Summary - **Overall Assessment:** The changes look good and accomplish the intended goal, with only a minor edge case around null-safety identified. - **Minor Suggestions:** - Add a guard check or exception handler around the return value of `fedfind.helpers.get_current_release()` to protect against potential `TypeError`. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*

adamwill commented

2026-04-10 00:49:40 +00:00

Author

Owner

Suggestion: Add a fallback or check to ensure curr is an integer before attempting arithmetic operations.

No need. get_current_release is expected to always return an integer. (I should really add type hints around here, one day...)

> Suggestion: Add a fallback or check to ensure curr is an integer before attempting arithmetic operations. No need. `get_current_release` is expected to always return an integer. (I should really add type hints around here, one day...)

adamwill merged commit e168f30282 into main

2026-04-10 00:50:55 +00:00

adamwill deleted branch fixup-fmw-handling

2026-04-10 00:50:55 +00:00

No reviewers

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

quality/python-wikitcms!15

No description provided.

Rows
Columns