Skip to content

tf 2 #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tf 2 #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 1 addition & 19 deletions thefuck_2/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import pytest
import warnings
from mock import Mock, call, patch
from mock import Mock, patch
Copy link

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

❓ Verification inconclusive

Consider restoring path separator test coverage.

According to the AI summary, tests for path separator behavior were removed. Given the critical platform compatibility issue in utils.py (hardcoded colon instead of os.pathsep), having test coverage for cross-platform PATH handling would be valuable to catch such regressions.

Let me check what path separator related tests were removed:

🏁 Script executed:

#!/bin/bash
# Search for any remaining path separator related tests
rg -n "pathsep|PATH.*split" --type py thefuck_2/tests/

Length of output: 56

Restore cross-platform PATH handling tests

Tests covering os.pathsep behavior appear to have been removed, and there’s currently no coverage to catch regressions in how we split or join PATH entries on different platforms. Please add back tests that verify:

  • Splitting a PATH-like string using os.pathsep
  • Joining list entries into a PATH string using os.pathsep
  • Handling edge cases (empty entries, trailing separators)

Suggested location for new tests:

  • thefuck_2/tests/test_utils.py
🤖 Prompt for AI Agents 
In thefuck_2/tests/test_utils.py around line 5, tests related to cross-platform
PATH handling using os.pathsep were removed, reducing coverage for platform
compatibility. Add back tests that verify splitting PATH-like strings by
os.pathsep, joining lists into PATH strings with os.pathsep, and handling edge
cases like empty entries and trailing separators. Place these tests in
test_utils.py to ensure robust coverage of path separator behavior across
platforms.

from thefuck.utils import default_settings, \
memoize, get_closest, get_all_executables, replace_argument, \
get_all_matched_commands, is_app, for_app, cache, \
Expand Down Expand Up @@ -76,24 +76,6 @@ def test_get_all_executables():
assert 'fuck' not in all_callables


@pytest.fixture
def os_environ_pathsep(monkeypatch, path, pathsep):
env = {'PATH': path}
monkeypatch.setattr('os.environ', env)
monkeypatch.setattr('os.pathsep', pathsep)
return env


@pytest.mark.usefixtures('no_memoize', 'os_environ_pathsep')
@pytest.mark.parametrize('path, pathsep', [
('/foo:/bar:/baz:/foo/bar', ':'),
(r'C:\\foo;C:\\bar;C:\\baz;C:\\foo\\bar', ';')])
def test_get_all_executables_pathsep(path, pathsep):
with patch('thefuck.utils.Path') as Path_mock:
get_all_executables()
Path_mock.assert_has_calls([call(p) for p in path.split(pathsep)], True)

Comment on lines 78 to -95
Copy link

@blar-app blar-app bot May 23, 2025

Choose a reason for hiding this comment

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

🟡 Info 🐛 Bug

Tests invoking get_all_executables run against real OS PATH and os.pathsep after removal of os_environ_pathsep fixture, causing non-determinism.

Issue Explanation
  • The 'os_environ_pathsep' fixture and associated parametrized test were removed.
  • The function test_get_all_executables calls get_all_executables without patching or monkeypatching os.environ['PATH'] or os.pathsep.
  • This causes test_get_all_executables to run against the real environment variables, risking non-deterministic or incorrect behavior on different platforms.
  • The previous fixture explicitly controlled PATH and pathsep for deterministic testing behavior, which is now missing.

Reply if you have any questions or let me know if I missed something.

Don't forget to react with a 👍 or 👎 to the comments made by Blar to help us improve.


@pytest.mark.parametrize('args, result', [
(('apt-get instol vim', 'instol', 'install'), 'apt-get install vim'),
(('git brnch', 'brnch', 'branch'), 'git branch')])
Expand Down
2 changes: 1 addition & 1 deletion thefuck_2/thefuck/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def _safe(fn, fallback):
tf_entry_points = ['thefuck', 'fuck']

bins = [exe.name.decode('utf8') if six.PY2 else exe.name
for path in os.environ.get('PATH', '').split(os.pathsep)
for path in os.environ.get('PATH', '').split(':')
Copy link

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Critical: Hardcoded path separator breaks Windows compatibility.

This change from os.pathsep to a hardcoded colon ':' will break functionality on Windows systems where PATH uses semicolon (;) as the separator. This makes the function Unix-specific and inconsistent with the which function (line 64) that correctly uses os.pathsep.

Apply this diff to restore cross-platform compatibility:

-            for path in os.environ.get('PATH', '').split(':')
+            for path in os.environ.get('PATH', '').split(os.pathsep)

Let me verify if there are other usages of PATH splitting in the codebase:

🏁 Script executed:

#!/bin/bash
# Search for other PATH splitting patterns in the codebase
rg -n "PATH.*split" --type py
rg -n "os\.pathsep" --type py

Length of output: 1980

Critical: Hardcoded path separator breaks Windows compatibility

The change at thefuck_2/thefuck/utils.py:121 switches from the portable os.pathsep to a hardcoded ':', making this function Unix-only and inconsistent with other code in this file.

Locations needing a fix:

  • thefuck_2/thefuck/utils.py, line 121

Apply this diff to restore cross-platform behavior:

-            for path in os.environ.get('PATH', '').split(':')
+            for path in os.environ.get('PATH', '').split(os.pathsep)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for path in os.environ.get('PATH', '').split(':')
for path in os.environ.get('PATH', '').split(os.pathsep)
🤖 Prompt for AI Agents 
In thefuck_2/thefuck/utils.py at line 121, the code uses a hardcoded colon ':'
to split the PATH environment variable, which breaks compatibility on Windows
where the separator is a semicolon ';'. Replace the hardcoded ':' with
os.pathsep to ensure the code works correctly across all platforms and remains
consistent with other parts of the codebase.

Copy link

@blar-app blar-app bot May 23, 2025

Choose a reason for hiding this comment

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

🔴 Error 🐛 Bug

Incorrect splitting of PATH environment variable on ':' breaks Windows path handling in executable search.

Issue Explanation
  • The code splits the PATH environment variable using ':' instead of os.pathsep.
  • On Windows, this breaks paths like C:\Program Files into invalid segments (C, \Program Files).
  • The code attempts to iterate these invalid segments as directories, causing silent skipping of executables.
  • This can cause missing executables from PATH on Windows systems.
  • The previous code correctly used os.pathsep for platform-independent splitting.

Reply if you have any questions or let me know if I missed something.

Don't forget to react with a 👍 or 👎 to the comments made by Blar to help us improve.

Copy link

@blar-app blar-app bot May 23, 2025

Choose a reason for hiding this comment

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

🟠 Warning 🐛 Bug

Hardcoded ':' used instead of os.pathsep for splitting PATH, breaking Windows support.

Issue Explanation
  • The changed code uses split(':') on PATH environment variable instead of split(os.pathsep).
  • On Windows, PATH entries are separated by ';', not ':', so splitting by ':' treats the entire PATH as a single entry.
  • This causes the bins list to be empty or missing executables on Windows.
  • The original code correctly used os.pathsep for platform-agnostic PATH splitting.
  • This change breaks executable discovery on Windows platforms.

Reply if you have any questions or let me know if I missed something.

Don't forget to react with a 👍 or 👎 to the comments made by Blar to help us improve.

for exe in _safe(lambda: list(Path(path).iterdir()), [])
if not _safe(exe.is_dir, True)
and exe.name not in tf_entry_points]
Expand Down