Skip to content

Conversation

@tobiashochguertel
Copy link

This PR provides additional fixes on top of PR #330 to achieve 100% Python 3.12 compatibility.

Related

Builds upon #330 by @mundanevision20

Summary

These additional Python 2→3 compatibility fixes are needed on top of PR #330 to achieve 100% test passing rate.

Changes

  • Fix iterator protocol: def next()def __next__() in Peekable class
  • Fix iterator calls: it.next()next(it) throughout codebase
  • Fix map() iterator: yield map()yield list(map()) in group_continuous
  • Fix dictionary methods: .iteritems().items() in doctests
  • Fix function names in doctests to match actual definitions
  • Migrate testing framework: nose → pytest (Python 3.12 compatible)

Why These Fixes Are Needed

  • nose is incompatible with Python 3.12 (requires removed imp module)
  • Iterator protocol requires __next__() in Python 3, not next()
  • map() returns iterators in Python 3, not lists
  • Doctests were failing due to Python 2 syntax

Testing

All 12 tests now passing (100% success rate):

pytest --doctest-modules tests/ explainshell/
============================== 12 passed in 0.37s ==============================

Tested with full database integration (72,349 documents) and verified with commands like ls -la, grep -r, tar -xzvf.

Files Changed

  • Makefile - Updated test command from nosetests to pytest
  • explainshell/options.py - Fixed .iteritems() → .items() in doctests
  • explainshell/util.py - Fixed iterator protocol and map() iterator
  • explainshell/web/views.py - Fixed iterator calls and function names
  • requirements.txt - Replaced nose with pytest

rugk and others added 22 commits May 16, 2019 09:22
…milar

Signed-off-by: Страхиња Радић <contact@strahinja.org>
- Fix iterator protocol: def next() → def __next__() in Peekable class
- Fix iterator calls: it.next() → next(it) throughout codebase
- Fix map() iterator: yield map() → yield list(map()) in group_continuous
- Fix dictionary methods: .iteritems() → .items() in doctests
- Fix function names in doctests to match actual definitions
- Migrate testing framework: nose → pytest (Python 3.12 compatible)
- Update requirements.txt with pytest 8.3.3 and pytest-cov 6.0.0
- Update Makefile to use pytest --doctest-modules

These fixes complete the Python 3.12 migration on top of PR idank#330.
All 12 tests now passing (100% success rate).

Test results:
- explainshell/algo/features.py: 1 passed
- explainshell/fixer.py: 1 passed
- explainshell/manpage.py: 3 passed
- explainshell/options.py: 3 passed
- explainshell/util.py: 3 passed
- explainshell/web/views.py: 1 passed

Total: 12 passed in 0.37s

Database integration verified with 72,349 documents.
Full functionality tested with ls, grep, and tar commands.
Copilot AI review requested due to automatic review settings October 26, 2025 14:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR completes the Python 2→3 migration by fixing iterator protocol methods, updating dictionary operations, and migrating from nose to pytest. The changes enable 100% test compatibility with Python 3.12.

Key Changes:

  • Fixed iterator protocol: def next()def __next__() and it.next()next(it)
  • Updated dictionary methods from .iteritems().items()
  • Migrated testing framework from nose to pytest
  • Fixed map() iterator behavior by wrapping with list()

Reviewed Changes

Copilot reviewed 46 out of 50 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
explainshell/util.py Fixed iterator protocol (__next__) and map() wrapping
explainshell/web/views.py Updated iterator calls and function references
explainshell/store.py Changed .iteritems() to .items()
explainshell/options.py Fixed doctests with .items()
tests/*.py Updated assertions from assertEquals to assertEqual
Makefile Changed test command from nosetests to pytest
requirements.txt Replaced nose with pytest (not shown in diffs)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

|| 'default';

if (!docCookies.getItem(themeCookieName)) {
var selectedTheme = 'default';
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

Variable selectedTheme is declared twice, with the second declaration at line 1154 overwriting the theme detection logic from lines 1149-1151. Remove the duplicate declaration and move the assignment inside the if block or remove the if block entirely if the first declaration is sufficient.

Suggested change
var selectedTheme = 'default';
selectedTheme = 'default';

Copilot uses AI. Check for mistakes.
# activate logging and redirect all logs to loguru
logging.basicConfig(handlers=[InterceptHandler()], level=logging.DEBUG, force=True)

if len(config.HOST_IP) > 1:
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

The condition checks if config.HOST_IP length is greater than 1, but based on the config file changes, HOST_IP is now set to an empty string by default instead of False. This condition should check for truthiness (non-empty string) rather than length greater than 1. Change to if config.HOST_IP: to properly handle empty string vs valid IP address.

Suggested change
if len(config.HOST_IP) > 1:
if config.HOST_IP:

Copilot uses AI. Check for mistakes.
@tobiashochguertel
Copy link
Author

Closing this PR - it accidentally included all commits from PR #330.

The additional Python 2→3 compatibility fixes have been documented and are available in a separate branch. These fixes (only 5 files changed) are needed on top of PR #330 to achieve 100% Python 3.12 compatibility and test passing rate.

I will add a comment on PR #330 with details about these fixes.

@tobiashochguertel tobiashochguertel deleted the python3-additional-fixes branch October 26, 2025 15:04
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.

4 participants