Skip to content

✨ Remove default API version and make it manual input#183

Merged
ponty33 merged 8 commits intomainfrom
feature/173
Jun 5, 2023
Merged

✨ Remove default API version and make it manual input#183
ponty33 merged 8 commits intomainfrom
feature/173

Conversation

@ponty33
Copy link
Copy Markdown
Member

@ponty33 ponty33 commented May 19, 2023

  • Remove the api_version in Token, I couldn't found a way to override the field value
  • Let api_version be input in execute_rest and execute_gql, since we construct full url inside the function
  • If no version is entered, default to /admin which the oldest supported version will be used
  • Modify test

close #173

@ponty33 ponty33 self-assigned this May 19, 2023
@ponty33 ponty33 marked this pull request as ready for review May 29, 2023 16:18
@ponty33 ponty33 requested review from hillairet and lishanl as code owners May 29, 2023 16:18
Copy link
Copy Markdown
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

I am very confused.aI thought I already commented on this! Did I forget to press send?

  1. Do you really want to have to pass on the api version on every single call??? It would be annoying and error prone when it's option! Image some calls being on one API version and others on another version!
  2. I don't understand why it is impossible to do Token.api_version = '2023-04'. 🤔

@ponty33
Copy link
Copy Markdown
Member Author

ponty33 commented May 30, 2023

I will answer 2 first, yes, it just doesn't work. There are something about ABC behavior that I don't under stand, but I couldn't make it work when writing test for it
If 2 is not doable, the only work around I can think of is enter it in the arg
If you use the same constant such as API_VERSION='2023-04' in the code you can ensure it's always using the same version in the code. I know it's not pretty, but since simply assign doesn't work then I don't know what else I can do.

@hillairet
Copy link
Copy Markdown
Collaborator

I don't see the problem here:

from abc import ABC, abstractmethod
from typing import ClassVar, Optional

from pydantic import BaseModel


class Token(ABC, BaseModel):
    store_name: str

    api_version: ClassVar[Optional[str]] = None


class OfflineTokenABC(Token, ABC):
    @abstractmethod
    def version(self):
        pass


class MyOfflineTokenABC(OfflineTokenABC):
    api_version: ClassVar[Optional[str]] = '2023-04'

    def version(self):
        print(self.api_version)


offline = MyOfflineTokenABC(store_name='potato')
print(offline.api_version)
offline.version()

MyOfflineTokenABC.api_version = '2024-04'
print(offline.api_version)
print(Token.api_version)

Output as expected:

2023-04
2023-04
2024-04
None

So two ways of setting the api_version.
Maybe I'm not testing your problem though ...

@ponty33
Copy link
Copy Markdown
Member Author

ponty33 commented Jun 1, 2023

I found where I did wrong...
When I wrote the test, I make the load function returning another class that doesn't have API version 😓
No wonder the version is nowhere to be found 😞

@ponty33 ponty33 requested a review from hillairet June 1, 2023 16:32
Copy link
Copy Markdown
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

Small things

Comment thread tests/admin_api/test_initialization.py Outdated
Comment thread tests/admin_api/test_initialization.py Outdated
Comment thread tests/admin_api/test_initialization.py Outdated
@ponty33 ponty33 requested a review from hillairet June 2, 2023 18:53
Copy link
Copy Markdown
Collaborator

@lishanl lishanl left a comment

Choose a reason for hiding this comment

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

🆒

Copy link
Copy Markdown
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

Neat

@ponty33 ponty33 merged commit 468dbce into main Jun 5, 2023
@ponty33 ponty33 deleted the feature/173 branch June 5, 2023 18:37
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.

Update default API version.

3 participants