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

Python 3.12 compat #463

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

auburus
Copy link

@auburus auburus commented Dec 18, 2024

Hi,

I took a stab at addressing #430, let me describe a little bit the changes because it ended up being a larger PR than what I intended.

The PR is split in 3 commits:

  1. Breaking change that made migrating to python's 3.12 pathlib much easier.
  2. Make code work and all unit tests pass under python3.12, but break when <= 3.11
  3. Make code work in 3.8 - 3.11, and fix tox/pre-commit when running under python3.12.

Why did I have to make a breaking change

First, I tried just upgrading to python 3.12 and working around the changes done in pathlib.PurePath, but I was having to copy/paste and slightly tweak a lot of methods because of the choice of using the first part of the path as the "root".

An example of what I mean:

# This should be true
ArtifactoryPath('http://b.com/artifactory/repo/path/file.txt').is_relative_to('http://b.com/artifactory') == False

This is because the second path doesn't have a root, but IMO this is a relative path.

So, to fix that I've made the first commit, which is changing in python3.11 and before the way the root is handled. This is how the behavior changes, and that may be a BREAKING change, so I am not sure if that is acceptable or not.

# Current behavior
_ArtifactoryFlavour().splitroot('http://b.com/artifactory/repo/folder/file.txt') == ('http://b.com/artifactory', '/repo/', 'folder/file.txt')

# New behavior
_ArtifactoryFlavour().splitroot('http://b.com/artifactory/repo/folder/file.txt') == ('http://b.com/artifactory', '/', 'repo/folder/file.txt')

I think this may not be so bad, since most of the interface is internal. If a code is using ArtifactoryPath.root this will change, if it is using ArtifactoryPath.repo it is backwards compatible.

Final comments

After that change, the migration was much easier. Commit 2 is making the code work in python3.12, and commit 3 is making it work at the same time in python3.12 AND in older python versions.

There are some small changes in precommit files that I had to do to make sure they would work when tox is invoked under python3.12, but I can drop those if needed, or open a separate PR.

Last thing: All unit tests pass, but I couldn't get a trial license to integration test those changes, so I didn't run that, sorry.

Let me know if I can do anything to help the review!

When checking the root in a UnixFlavour system, it is / for an absolute
path, and '' for a relative one, and unix systems have no drive.

For artifactory, if we consider the drive to be "http://b/artifactory"
and root to be "/", it behaves exactly like Windows and Unix Flavours,
and no if's and exceptions to handle the root are needed in the code.

This refactor makes the migration to python 3.12 much easier, since we
can subclass pathlib.Path directly and we don't need to override as many
methods.

Notice: This is a breaking change if you are accessing the `root` property
directly, but I'm assuming that mostly everyone should be using the
`repo` accessor for that.
There is also some minor changes in leading/trailing slashes
Also, if tox is installed with python3.12, some pre-commit plugins
didn't work, so I have updated those.
@auburus auburus changed the title Python 312 compat Python 3.12 compat Dec 18, 2024
Copy link
Member

@allburov allburov left a comment

Choose a reason for hiding this comment

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

May be insted of defining many "if" across code, we can do something like inheretance (or even duplicating classes

_ArtifactoryPath(...):
   ... # define <3.11 and comment why it's this way

_ArtifactoryPath311(_ArtifactoryPath):
   ... # define <3.11 and comment why it's this way

BaseClass = _ArtifactoryPath if IS_PYTHON_3_12_OR_NEWER else _ArtifactoryPath311
ArtifactoryPath(BaseClass)

This way when 3.11 ends it's lifecycle (31 Oct 2027) - we can just remove the whole _ArtifactoryPath311 and keep using new version

So the idea is to move all 3.11 related code to a separate class and add new things to 3.12 class (but we can name it as regular class, 312 not required in it's name, because it'll be only class soon)

artifactory.py Outdated Show resolved Hide resolved
artifactory.py Outdated Show resolved Hide resolved
artifactory.py Outdated Show resolved Hide resolved
Comment on lines +512 to +513
root = sep
rest = sep.join(path_parts[0:])
Copy link
Member

Choose a reason for hiding this comment

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

Are all these changes needed for Python 3.12 compatibility?

Can we limit the PR to just the necessary updates? This will make it easier to debug if it causes any regression issues.

Copy link
Author

Choose a reason for hiding this comment

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

I tried addressing this in the PR description because I completely understand this might be a blocker. Let me try explaining it again, and I'll be happy to try any suggestion you may have to avoid this set of changes (I couldn't think of one, apologies).

When I tried just doing the changes for 3.12 compatibility, one of the tests that broke was the one that relied on this being true:

ArtifactoryPath('http://b.com/artifactory/repo/path/file.txt').is_relative_to('http://b.com/artifactory')

This statement returns true in python3.11 but returns false in python3.12, and my guess is that we are relying on undefined behavior, based on how PureWindowsPath works.

# python 3.12
>>> PureWindowsPath("C://abc/def.txt").is_relative_to("C:")
False

So I felt I had two choices: Override most PurePath methods for artifactory, so it would behave as it used to in python3.11, or change ArtifactoryPath to consider / as root, instead of the repo. I thought the first option would be a maintenance nightmare, so that is why I went with the second one, but I could be wrong.

Happy to hear your thoughts on the subject!

(Also, all the changes for this new behavior are part of the first commit, happy to push that as a separate pull request if you think it would make things easier)

This commit addresses the concerns raised in the Pull request. It helps minimize
the amount of changes (way less indentation) compared to master.

Also, used a single constant for marking code that will be removed once 3.11 is
deprecated.

This commit likely should be "merged" into the previous two once the PR review
process is over. And if not, well, you have this nice comment here forever :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants