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

Remove @property implementations for field access #262

Closed
tdstein opened this issue Sep 4, 2024 · 0 comments
Closed

Remove @property implementations for field access #262

tdstein opened this issue Sep 4, 2024 · 0 comments
Labels
breaking This is a breaking change enhancement New feature or request sdk Used for automation
Milestone

Comments

@tdstein
Copy link
Collaborator

tdstein commented Sep 4, 2024

Note: this change requires updating the cookbook documentation. issue forthcoming...

The initial intention of using the @property annotation for field access was to provide compatibility between Connect versions without having to modify code using posit-sdk. Since then, I've yet to encounter a field whose name has changed between Connect versions. Therefore implementing property methods has introduced a lot of cruft in the codebase. Additionally, we often have to mark getters with # type: ignore to ensure compatibility, which is counterintuitive.

An alternative solution to compatibility between Connect versions is to override the getitem and setitem methods as shown below. This is a simple approach and much more concise than our current implementation.

import warnings


class CompatibleDict(dict):
    deprecated_keys = {"old": "new"}

    def __getitem__(self, key):
        # Check if the key is deprecated and use the new key if necessary
        if key in self.deprecated_keys:
            new_key = self.deprecated_keys[key]
            print(f"Warning: '{key}' is deprecated, use '{new_key}' instead.")
            key = new_key
        return super().__getitem__(key)

    def __setitem__(self, key, value):
        if key in self.deprecated_keys:
            new_key = self.deprecated_keys[key]
            warnings.warn(f"Warning: '{key}' is deprecated, use '{new_key}' instead.")
            key = new_key
        super().__setitem__(key, value)

Since this is breaking change, I propose a two-phased approach..

Phase 1 (next minor release e.g., v.0.5.0)

Remove all property methods for field access and implement getattr and setattr on Resource as follows. This implementation should also include a deprecation warning.

    def __getattr__(self, name):
        # Check if the name exists as a key in the dictionary
        if name in self:
            return self[name]
        # If the name does not exist as a key, raise the AttributeError
        raise AttributeError(f"'{self.__class__.__name__}' object has no attribute '{name}'")

    # Optional: __setattr__ to support setting attributes
    def __setattr__(self, name, value):
        self[name] = value

Phase 2 (next major release e.g., v1.0.0)

Remove the getattr and setattr implementations defined above.

@tdstein tdstein added enhancement New feature or request breaking This is a breaking change labels Sep 4, 2024
@tdstein tdstein added this to the 1.0.0 milestone Sep 4, 2024
@tdstein tdstein added the sdk Used for automation label Sep 13, 2024
tdstein added a commit that referenced this issue Sep 20, 2024
BREAKING CHANGE: removes support for accessing resource fields using
`__getattr__` (e.g., `user.username`). Use `__getitem__` instead (e.g.,
`user["username"]`.

depends on #265 and #266
closes #262
@tdstein tdstein closed this as completed Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change enhancement New feature or request sdk Used for automation
Projects
None yet
Development

No branches or pull requests

1 participant