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

Drop Python 3.7 and add Python 3.12 #112

Merged
merged 27 commits into from
Jan 11, 2024
Merged

Drop Python 3.7 and add Python 3.12 #112

merged 27 commits into from
Jan 11, 2024

Conversation

justindujardin
Copy link
Owner

@justindujardin justindujardin commented Jan 10, 2024

The circle of life is beautiful. Python 3.7 has reached its end, and Python 3.12 rises to carry on its name.

Motivation
Pathy has relied on the internals of pathlib.Path to support complex things like glob matching and the differences between Windows and Posix paths. Because of this when the python developers rightly refactor their internals, Pathy breaks. 😭

We need to stop depending on the internals of pathlib to avoid the yearly "new version of python breakage". It was on my list to inline the parts of pathlib needed and call it good, but then the absolute legend @barneygale published a pypi package pathlib_abc that's synced from CPython and implements the logic we depended on inside of pathlib.

Now, we can have a proper public API to work from and not lose sleep every time a new version of Python is released. 😎

This PR updates Pathy to use pathlib_abc and supports Python 3.12 while deprecating Python 3.7, which reached end-of-life in the middle of last year.

Changes

Breaking Changes

  • Pathy no longer inherits from pathlib.Path changing type checks looking at a Pathy comparing it for pathlib.Path. This is better overall but could break user code in rare cases.
  • Pathy no longer raises a ValueError when given an invalid bucket path. It's unclear how to cleanly differentiate file system paths from chunks of paths used for constructing bucket paths, except in the absolute path case.
  • Pathy no longer automatically collapses relative paths. Before Pathy('gs://bucket/foo/../bar') would automatically become Pathy('gs://bucket/bar'). Now you have to use resolve() to get the same output Pathy('gs://bucket/foo/../bar').resolve()

BREAKING CHANGE: This drops support for python 3.7 which has reached its end of life
 - dirty first pass, validating against the FS adapter test suite.
 - 38 fail / 92 passed so far
 - test_pathy, test_pure_pathy, test_cli left for FS adapter

BREAKING CHANGE: Pathy.key returns a str rather than a Pathy instance

There was nowhere in the code where I wasn't casting the .key access to string. This removes all those casts and saves a little effort. It's possible that some user expects .key to be a Path still, so add a breaking change.
 - still need to check tests against cloud adapters
 - this disabled some tests and functionality, but nothing in terms of the major public API
 - old Pathy inherited some local file behaviors from pathlib.Path and those are now gone. This is a breaking change, but is unlikely to impact most use-cases.

BREAKING CHANGE: Pathy no longer inherits from pahtlib.Path

This means Pathy does not support directly accepting and working with file system paths. You must use Pathy.fluid or pathlib.Path to construct your file system paths. Pathy will continue to interoperate with them as needed to accomodate its public API.
 - remove as_uri for gcs paths.
 - fix sep access since _flavour is no more :sun:
@justindujardin justindujardin changed the title feat(pypi): Drop Python 3.7 and add Python 3.12 Drop Python 3.7 and add Python 3.12 Jan 10, 2024
 - auto format removed apparently unused posixpath imports. Alias them directly
 - implement is_symlink to stop the base class from continuing down a bad path
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (90eb026) 99.91% compared to head (fc85a5c) 99.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   99.91%   99.91%   -0.01%     
==========================================
  Files           6        7       +1     
  Lines        1159     1122      -37     
==========================================
- Hits         1158     1121      -37     
  Misses          1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- use a more foolproof invocation of bdist_wheel: https://stackoverflow.com/a/70459530/287335
- resolve build package warning about wanting underscore case keys
 - derp couldn't ever be hit given the preceeding code
 - I'll reapply it in another PR where it doesn't skew the diff stats so much
 - Pathy wants to provide a consistent API when using pathlib Paths or Bucket paths. To that end we extend pathlib.Path for all POSIX/Windows paths returned from pathy (usually in .fluid()) to add ls()
 - drop some dead code and remove some todos
@justindujardin justindujardin marked this pull request as ready for review January 11, 2024 18:47
 - we can update it as needed

_flavour = _PathyFlavour()
pathmod = pathy_pathmod
Copy link

Choose a reason for hiding this comment

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

@barneygale this will be a problem, right?

Copy link
Owner Author

@justindujardin justindujardin Jan 11, 2024

Choose a reason for hiding this comment

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

@merwok thanks for the review!

Can you tell me why you think this will be a problem? I'm happy to fix something if it's wrong.

This code is exercised a bunch by the test suite, and the behaviors are as expected across Python (3.8-3.12), so hopefully it's not too wrong.

I assumed that the introduction of pathmod here was to make it more easily accessible to the path classes (e.g., you can say some_path.pathmod.sep and get the proper separator to use.

Copy link

Choose a reason for hiding this comment

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

Oh, because this attribute is not public in pathlib anymore: python/cpython#100502 (comment)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I understand, thanks. 🙇

I removed the assignment of pathmod with Pathy's specific module, but it caused cascading failures in my test suite.

The trouble is that the pathmod defines how to split the string for root/drv/tail, and without it, my bucket paths get mangled.

If we shouldn't override pathmod, we'll need another way to control the path-splitting logic.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because this is functional and fixes our broken main branch build, I will merge this now.

Please feel free to comment here or in another issue and we can refine this implementation where needed.

Choose a reason for hiding this comment

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

@merwok pathmod is still public! You were right to urge me to keep it, as it rather nicely abstracts away all the pure functionality of the path using composition rather than inheritance.

I'm thinking of adding a PathModuleBase to define its interface: #113893

Copy link

@merwok merwok Jan 12, 2024

Choose a reason for hiding this comment

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

It is? Thought it was added in 3.13 then removed (so renamed to _pathmod in the end)
Edit: mixed up the commit on your repo and PRs to upstream!

Choose a reason for hiding this comment

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

I considered renaming it to _pathmod but decided against it, given your advice to wait: python/cpython#100502 (comment)

@justindujardin justindujardin merged commit 97ebaa1 into master Jan 11, 2024
8 checks passed
@justindujardin justindujardin deleted the feat/python312 branch January 11, 2024 22:52
github-actions bot pushed a commit that referenced this pull request Jan 11, 2024
# [0.11.0](v0.10.3...v0.11.0) (2024-01-11)

### Features

* Drop Python 3.7 and add Python 3.12 ([#112](#112)) ([97ebaa1](97ebaa1))

### BREAKING CHANGES

* This drops support for python 3.7 which has reached its end of life

* feat(Pathy): integrate pathlib_abc
 - replace base pathlib.Path class with abstract base class from a future version of python.
* Pathy.key returns a str rather than a Pathy instance

* feat(ci): add python 3.12
* Pathy no longer inherits from pahtlib.Path

This means Pathy does not support directly accepting and working with file system paths. You must use Pathy.fluid or pathlib.Path to construct your file system paths. Pathy will continue to interoperate with them as needed to accommodate its public API.
Copy link

🎉 This PR is included in version 0.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.12 compatibility Heads up: pathlib.VirtualPath may be coming soon
3 participants