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

Add presence tracking to structure design #49

Closed
wants to merge 1 commit into from

Conversation

JordonPhillips
Copy link
Contributor

This will be needed due to smithy-lang/smithy#1286

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@JordonPhillips JordonPhillips requested a review from a team as a code owner June 27, 2022 15:12
@JordonPhillips JordonPhillips changed the title Add presence tracking to structures Add presence tracking to structure design Jun 27, 2022
designs/shapes.md Outdated Show resolved Hide resolved
@JordonPhillips JordonPhillips force-pushed the custom-defaults-design branch 3 times, most recently from 5d17a65 to 2ed3865 Compare June 28, 2022 15:50
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Two small comments, otherwise things look good!

designs/shapes.md Outdated Show resolved Hide resolved
designs/shapes.md Show resolved Hide resolved
@JordonPhillips JordonPhillips requested review from nateprewitt and a team October 17, 2022 15:41
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

I think this looks good for the code changes. It looks like both Pants and the Python Codegen build are failing though. Do we know what's going on there?


# We need these various default types to be able to track whether a thing was
# set explicitly or not. These will be generated for every service.
class _DEFAULT_INT(int):
Copy link
Member

Choose a reason for hiding this comment

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

Are these sentinel values? What about just adding a dict that track which values were explicitly set vs which were defaulted?

Copy link
Contributor Author

@JordonPhillips JordonPhillips Oct 18, 2022

Choose a reason for hiding this comment

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

They're just wrapper classes. It's not exactly a sentinel because they do actually have values.

Using a tracking dict is problematic for a few reasons. Probably the biggest is that there's a lot of ways to modify an object's attributes so making that airtight is difficult. They could, for instance, just modify the __dict__ of the object directly and thus bypass __setattr__. Maybe that's a far-fetched scenario to account for, but it is still a problem we don't need to have.

It also means that you have to set the default value to None in the object definition everywhere since there's no way to distinguish between a defaulted value and an explicitly set one. This means that the type for the member is now int | None instead of int for example, which sort of defeats one of the intents of having defaults. This also makes auto-docs like the kind that show up during dot-completion worse since they can't show you the real default value now. Actually now that I think of it, it would also be impossible to distinguish between an explicit and implicit None for document types.

It also just takes a lot more code. See below for a simplified example. While that's not quite as pressing an issue for python as for, say, JavaScript, it still is an issue.

class WithWrappers:
    def __init__(
        self,
        *,
        default_int: int = _DEFAULT_INT(7),
        default_str: str = _DEFAULT_STR("foo")
    ):
        self.default_int = default_int
        self.default_str = default_str


class WithoutWrappers:
    def __init__(
        self, *, default_int: int | None = None, default_str: str | None = None
    ):
        self._has = {}
        if default_int is None:
            self.default_int = 7
            self._has["default_int"] = False
        else:
            self.default_int = default_int
            self._has["default_int"] = True

        if default_str is None:
            self.default_str = "foo"
            self._has["default_str"] = False
        else:
            self.default_str = default_str
            self._has["default_str"] = True

    def __setattr__(self, name: str, value: Any) -> None:
        object.__setattr__(self, name, value)
        self._has[name] = True

    def _hasattr(self, name: str) -> bool:
        return self._has[name]


print(inspect.signature(WithWrappers.__init__))
# (self, *, default_int: int = 7, default_str: str = 'foo')


print(inspect.signature(WithoutWrappers.__init__))
# (self, *, default_int: int | None = None, default_str: str | None = None)

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good. Why generate these for each service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why generate these for each service?

We don't want people to use them, so they need to be private.

@JordonPhillips
Copy link
Contributor Author

I think this looks good for the code changes. It looks like both Pants and the Python Codegen build are failing though. Do we know what's going on there?

There's a separate PR to get the build passing on latest.

@JordonPhillips
Copy link
Contributor Author

JordonPhillips commented Oct 18, 2022

Hmm.... I forgot you can't subclass None or bool so wrapping may not work ideally either. :/

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.

3 participants