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

flit_core: refactor path handling to use pathlib #516

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

Conversation

jameshilliard
Copy link
Contributor

This should make path handling easier/cleaner compared with using os.path.

flit_core/flit_core/common.py Outdated Show resolved Hide resolved
flit_core/flit_core/common.py Outdated Show resolved Hide resolved
flit_core/flit_core/buildapi.py Outdated Show resolved Hide resolved
@@ -201,18 +231,18 @@ def _check_glob_patterns(pats, clude):
.format(clude, p)
)

normp = osp.normpath(p)
normp = posix_normpath(PurePosixPath(p))
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a behaviour change on Windows -- since it's switching from Windows paths to POSIX paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The path handling seemed buggy/inconsistent here since os.path.normpath is filesystem/os specific and these validations I think need to be consistent across platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests already expect these paths to always be posix style at the moment, as this path is not an absolute path on windows:

('/home', 'absolute path'),

@pradyunsg
Copy link
Member

I'm not sure that it is particularly useful to make such a migration, but I'll leave the merits of thinking about those to others.

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