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

Implemented a version control #117

Closed
wants to merge 1 commit into from
Closed

Implemented a version control #117

wants to merge 1 commit into from

Conversation

khsrali
Copy link
Contributor

@khsrali khsrali commented Jul 31, 2024

Fixes #116

This way, when new functionalities are added, simply by adding a decorator on top one can have a version control.

Note: no need to add this decorator for all methods, as the extra check may reduce performance.
Once support for previous api_version is no longer provided, decorator on each function can safely be removed one by one.

In addition I suggest to drop support for api < "v1.15.0", unless they are still deployed somewhere that I'm not aware of.

@khsrali
Copy link
Contributor Author

khsrali commented Jul 31, 2024

If you don't want to drop support for api_versions < "v1.15.0". Then we have to add more staff here:

missing_api_features = {
    '1.15.0': {
        'list_files': ['recursive'],
        'compress': ['ALL'],
        'extract': ['ALL'],
        'submit_compress_job': ['ALL'],
        'submit_extract_job': ['ALL']
    },
    '1.16.0': {},
}

Also need to decorate the respective functions, for instance: (otherwise the check is ignored)

@validate_api_version_compatibility()
    def extract(..

will always raise when v1.15.0, because 'ALL' means the entire method is not available.

while for example

@validate_api_version_compatibility(recursive=True)
    def list_files(..

would only raise if list_files is called explicitly with recursive=True in 1.15.0. Otherwise functions normally.

@ekouts ekouts self-requested a review August 23, 2024 14:15
@ekouts
Copy link
Collaborator

ekouts commented Oct 15, 2024

Sorry for the (very) late reply @khsrali 😬. I pulled your changes in #128 and the client should also query automatically the version from the api, when possible.
Thanks for the contribution! I decided that it was easier to just check the recurse argument in the client, instead of checking the named and positional arguments for ls. Your initial implementation would miss the case that recurse was passed as a positional argument and it wasn't too nice when I tried to have a "generic" solution.

@ekouts ekouts closed this Oct 15, 2024
@khsrali
Copy link
Contributor Author

khsrali commented Oct 15, 2024

Hi @ekouts no worries :)
Thanks a lot for pulling from it.

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.

A version control could be used across the interface
2 participants