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

Implement a custom __repr__ #328

Open
schloerke opened this issue Nov 8, 2024 · 5 comments
Open

Implement a custom __repr__ #328

schloerke opened this issue Nov 8, 2024 · 5 comments
Labels
sdk Used for automation

Comments

@schloerke
Copy link
Collaborator

          No, the following expression must be true, which it is not: `eval(repr(repository)) == repository`.

Please use the default methods from dict for the time being. In the future, we can look into implementing true __repr__ which would provide true serliaization functionality if it is necessary. See https://docs.python.org/3/library/functions.html#repr

Originally posted by @tdstein in #300 (comment)

@github-actions github-actions bot added the sdk Used for automation label Nov 8, 2024
@schloerke
Copy link
Collaborator Author

Ideas:

  • Use the dict's repr method
  •      def __repr__(self) -> str:
              items = super().items()
              args = ", ".join(f"{k}={repr(v)}" for k, v in items)
              return repr(f"ApiDictEndpoint({repr(self._ctx)}, {repr(self._path)}, {args})")
    ApiDictEndpoint({}, '/me', email='[email protected]', username='barret', first_name='Barret', last_name='Schloerke', password='', user_role='publisher', created_time='2018-04-25T19:03:10.669201Z', updated_time='2024-10-31T16:06:54.43695Z', active_time='2024-11-08T21:30:01.62323Z', confirmed=True, locked=False, guid='123', privileges=['add_vanities', 'publish_apps', 'remove_vanities'])
    

Notes:

  • Should this also be applied to Context or other classes? Currently its repr is {}. Suggested change which inherits from object as dict isn't leveraged:
class Context:
    session: requests.Session
    url: Url
    _version: str

    def __init__(self, session: requests.Session, url: Url):
        self.session = session
        self.url = url
        # self._version is retrieved on demand of `.version` property

    @property
    def version(self) -> Optional[str]:
        try:
            return self._version
        except AttributeError:
            endpoint = self.url + "server_settings"
            response = self.session.get(endpoint)
            result = response.json()
            value = self._version = result.get("version")
        return value

    @version.setter
    def version(self, value):
        self._version = value

@tdstein
Copy link
Collaborator

tdstein commented Nov 8, 2024

My understanding is that implementing __repr__ is only necessary when supporting serialization/deserialization is necessary. However, according to the Python documentation, it should be implemented whenever possible.

side note; using class variables this way is incorrect.

class Context:
    session: requests.Session
    url: Url
    _version: str

This implies that these variables are shared across all instances of the class. Since we support creating multiple Client instances (see the cookbook on blue/green deployments), we need to create multiple instances of Context with different session and URL values.

@tdstein
Copy link
Collaborator

tdstein commented Nov 8, 2024

Thinking through this a bit more. I'm not convinced implementing repr in this way is worth the effort. My opinion is that the current implementations using dict, and the implementation on ActiveSequence is sufficient for the purposes of serialization. For example, someone wants to query a list of users from Connect and then serialize the information for later. When deserializing, do they need instances of Users? Or is the list of dictionaries is enough? I would argue the list of instances list of dictionaries is enough for any downstream uses cases.

@jonkeane
Copy link
Collaborator

Thanks for all the context and details here. Very much agree that if there's doubt about if we'll need it, we should continue to build out the SDK so that we can gather more experience + information on if it's needed.

When deserializing, do they need instances of Users? Or is the list of dictionaries is enough? I would argue the list of instances is enough for any downstream uses cases.

I might not be following but in the last sentence here did you mean list of dictionaries instead of list of instances?

@tdstein
Copy link
Collaborator

tdstein commented Nov 12, 2024

I might not be following but in the last sentence here did you mean list of dictionaries instead of list of instances?

Yes, I meant "list of dictionaries". Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk Used for automation
Projects
None yet
Development

No branches or pull requests

3 participants