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: fix type hints for delete_existing_files() #96

Merged
merged 2 commits into from
May 15, 2024

Conversation

jameslamb
Copy link
Member

Contributes to #87

Resolves these errors from mypy:

_rapids_dependency_file_generator.py:32: error: Value of type variable "AnyStr" of "walk" cannot be "Union[PathLike[Any], Any]"  [type-var]
_rapids_dependency_file_generator.py:33: error: Need type annotation for "fn"  [var-annotated]
_rapids_dependency_file_generator.py:33: error: Argument 1 to "filter" has incompatible type "Callable[[Any], Any]"; expected "Callable[[Union[PathLike[Any], Any]], TypeGuard[Never]]"  [arg-type]
_rapids_dependency_file_generator.py:33: error: Item "PathLike[Any]" of "Union[PathLike[Any], Any]" has no attribute "endswith"  [union-attr]

os.PathLike is not intended to be used as a generic type hint wherever you might have a string that represents a filepath. It has a different, very specific purpose. From https://docs.python.org/3/library/os.html#os.PathLike

An abstract base class for objects representing a file system path, e.g. pathlib.PurePath.

delete_existing_files() is always passed a Python string, so it's type hint should be str.

parser.add_argument(
"--clean",
nargs="?",
default=None,
const="",
help=(
"Delete any files previously created by dfg before running. An optional "
"path to clean may be provided, otherwise the current working directory "
"is used as the root from which to clean."
),
)

if args.clean == "":
args.clean = os.path.dirname(os.path.abspath(args.config))

if args.clean:
delete_existing_files(args.clean)

import os

p = os.path.dirname(os.path.abspath("README.md"))

type(p)
# <class 'str'>

isinstance(p, os.PathLike)
# False

How I tested this

mypy \
    --ignore-missing-imports \
    --explicit-package-bases \
    ./src

@jameslamb jameslamb added bug Something isn't working non-breaking Introduces a non-breaking change labels May 14, 2024
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Maybe we can make fewer, larger PRs to cut down on the number of reviews? It seems like there are quite a few of these typing-related changes and doing them two lines at a time seems slow.

@jameslamb
Copy link
Member Author

Sure, I'll start making them a bit bigger. Was trying to keep them small and focused so they'd be easier to review but I might not have found the right balance there.

After this + #94 are merged I think this typing work could be finished in just 1 or maybe 2 more PRs.

@jameslamb jameslamb merged commit 97ed1ba into rapidsai:main May 15, 2024
4 checks passed
@jameslamb jameslamb deleted the mypy/delete-files-hint branch May 15, 2024 13:49
GPUtester pushed a commit that referenced this pull request May 15, 2024
## [1.13.9](v1.13.8...v1.13.9) (2024-05-15)

### Bug Fixes

* fix type hints for delete_existing_files() ([#96](#96)) ([97ed1ba](97ed1ba))
@GPUtester
Copy link

🎉 This PR is included in version 1.13.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

difyrrwrzd added a commit to difyrrwrzd/dependency-file-generator that referenced this pull request Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants