devopshq / artifactory

dohq-artifactory: a Python client for Artifactory
https://devopshq.github.io/artifactory/
MIT License
269 stars 137 forks source link

Allow cases in ArtifactoryPath.glob() pattern #429

Open dogbert911 opened 11 months ago

dogbert911 commented 11 months ago

Issue #428

beliaev-maksim commented 10 months ago

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

allburov commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

Let's release 1.0.0 than with that change and the previous one https://github.com/devopshq/artifactory/pull/422!

briantist commented 10 months ago

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 commented 4 months ago

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, https://github.com/python/cpython/pull/102710

beliaev-maksim commented 4 months ago

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 commented 4 months ago

:+1: 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