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

Allow cases in ArtifactoryPath.glob() pattern #429

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

Conversation

dogbert911
Copy link

Issue #428

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.

I'm a bit concerned about scenarios that rely on this behavior right now...
But overall it looks reasonable to fix that.
Thank you for the contribution!

@beliaev-maksim
Copy link
Member

I think it is a backwards incompatible change. Can we provide an option for case sensitive/insensitive?

@allburov
Copy link
Member

The problem is that glob interface doesn't support any argument there https://docs.python.org/3/library/pathlib.html#pathlib.Path.glob

The current implementation clearly has a bug - default Path.glob is case sensitive.
I agree that it can break someone script tho...

May be we can make introduce some flag for that in ArtifactoryPath, warning if it has default value and later we can switch it to True?

# By default, to keep the current behavior
ArtifactoryPath.GLOB_CASE_SENSITIVE = None

if ArtifactoryPath.GLOB_CASE_SENSITIVE is None:
    warning("It's bug, not a feature, please either set GLOB_CASE_SENSITIVE to False explicitly or it'll change to Case Sensitive glob in 0.11 release")

sensitive = GLOB_CASE_SENSITIVE is True
# call right glob

@dogbert911 @beliaev-maksim what do you think about it?

@beliaev-maksim
Copy link
Member

I think I am good with proposal

However, based on semantic versioning we need to make breaking changes only in major bump, eg 1.x.x

@dogbert911
Copy link
Author

@allburov Feel free to modify this PR as you need.
As for me, this PR fixes bug, so you can add some flag to support backward compatibility for a few next minor versions, and then remove this flag as deprecated

@briantist
Copy link
Contributor

I think I am good with proposal

However, based on semantic versioning we need to make breaking changes only in major bump, eg 1.x.x

According to semver, breaking changes can be made in minor versions when the major version is 0, so since this project is still in zerover, it can be done.

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

I do agree (as someone with a project that heavily depends on this one) that it would be better to announce/warn for at least 1 release before making the change.

We may also want to consider that if we're quite worried about backward compatibility, perhaps we should be at major version 1 already.

If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you’re worrying a lot about backward compatibility, you should probably already be 1.0.0.

@allburov
Copy link
Member

Let's release 1.0.0 than with that change and the previous one #422!

@briantist
Copy link
Contributor

briantist commented Aug 29, 2023

Let's release 1.0.0 than with that change and the previous one #422!

I might recommend releasing v0.10.0 with #422 (and anything else that fits), with an announcement about the upcoming breaking change in this PR, then releasing v1.0.0 with this PR after that. It's nice to have (known) breaking changes announced.

But I'd be fine with v1.0.0 as well.

@zhan9san
Copy link
Contributor

The problem is that glob interface doesn't support any argument there https://docs.python.org/3/library/pathlib.html#pathlib.Path.glob

The current implementation clearly has a bug - default Path.glob is case sensitive. I agree that it can break someone script tho...

May be we can make introduce some flag for that in ArtifactoryPath, warning if it has default value and later we can switch it to True?

# By default, to keep the current behavior
ArtifactoryPath.GLOB_CASE_SENSITIVE = None

if ArtifactoryPath.GLOB_CASE_SENSITIVE is None:
    warning("It's bug, not a feature, please either set GLOB_CASE_SENSITIVE to False explicitly or it'll change to Case Sensitive glob in 0.11 release")

sensitive = GLOB_CASE_SENSITIVE is True
# call right glob

@dogbert911 @beliaev-maksim what do you think about it?

It is supported in this PR, python/cpython#102710

@beliaev-maksim
Copy link
Member

Alternative way

We can introduce module variable, eg artifactory.glob_casesensitive=False, that preserves defaults

Add deprecation warning if user doesn't update the value

In two versions from now we change the defaults

@allburov
Copy link
Member

👍 for adding it on class, so people can use both variances in the same code and compare the results

# by default - let's use explicitly False
ArtifactoryPath.GLOB_CASE_SENSITIVE = False

path_with_case_insensitive = ArtifactoryPath()

path_with_case_sensitive = ArtifactoryPath()
path_with_case_sensitive.GLOB_CASE_SENSITIVE = True

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.

5 participants