Skip to content
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.

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

Fix incorrect usage of lstrip bug in fileutils tests #377

Merged
merged 3 commits into from
Dec 31, 2024

Conversation

theartful
Copy link
Contributor

@theartful theartful commented Dec 30, 2024

I'm using Arch linux (typical starting statement of an arch user lol), and one of the packages I use depend on boltons (https://aur.archlinux.org/packages/python-boltons). I faced an issue with tests failing causing me to do some manual intervention.

On my machine, I have

BOLTONS_PATH = "/home/theartful/.cache/yay/python-boltons/src/boltons/boltons"

The test checks that "fileutils.py" can be found using iter_find_files, and indeed it is part of iter_find_files(BOLTONS_PATH, patterns=['*.py']) as:

p = "/home/theartful/.cache/yay/python-boltons/src/boltons/boltons/fileutils.py"

The bug here is that "lstrip" doesn't remove the prefix, but instead removes all characters from the left that is part of the string. For example:

>>> BOLTONS_PATH = "/home/theartful/.cache/yay/python-boltons/src/boltons/boltons"
>>> p = "/home/theartful/.cache/yay/python-boltons/src/boltons/boltons/fileutils.py"
>>> p.lstrip(BOLTONS_PATH)
 ileutils.py

@mahmoud
Copy link
Owner

mahmoud commented Dec 30, 2024

Great catch, sincere thanks from a reformed Gentoo/Arch user. This is a classic bug, but we have to do a bit more to make the tests pass on older Pythons and Windows.

Here's a removeprefix that you can just drop in the file and use, or add to strutils if you're so inclined:

def removeprefix(text: str, prefix: str) -> str:
    if text.startswith(prefix):
        return text[len(prefix):]
    return text

As for Windows, the file path separator is throwing off tests, so we still need to strip that out with .lstrip(os.path.sep)

Looking forward to merging this, thanks again!

@theartful
Copy link
Contributor Author

I added removeprefix to strutils.py. I forgot all about windows and their usage of backslashes instead of forward slashes.

@mahmoud mahmoud merged commit 41c8cbd into mahmoud:master Dec 31, 2024
10 checks passed
@mahmoud
Copy link
Owner

mahmoud commented Dec 31, 2024

Looks great! Thanks so much again!

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.

2 participants