Skip to content

Conversation

@Fokko
Copy link
Contributor

@Fokko Fokko commented Jun 9, 2022

Instead of having a specialized caching mechanism per class, we can generalize it as a singleton metaclass that we can reuse across different classes.

This moves the Singleton out of the types.py and implements it as a singleton.

Changing Class(ABC) to Class(metaclass=ABCMeta) is equivalent, but nicer since it more explicitly shows that we're setting a metaclass. From abc:

class ABC(metaclass=ABCMeta):
    """Helper class that provides a standard way to create an ABC using
    inheritance.
    """
    __slots__ = ()

@github-actions github-actions bot added the python label Jun 9, 2022
Instead of having specialized caching mechanism, we can generalize
it is a singleton metaclass that we can reuse across different
classes
@Fokko Fokko force-pushed the fd-use-a-singleton branch from 5e49358 to 9bd5312 Compare June 9, 2022 18:44


class Literal(Generic[T], ABC):
class Literal(Generic[T], metaclass=ABCMeta):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want the reader to know that this is changing a metaclass and not just inheriting from ABC? It seems like bringing a metaclass into the code is more clear about what's happening, but introduces a distinction that people (like me) may not understand or need.

@rdblue
Copy link
Contributor

rdblue commented Jun 12, 2022

Looks fine to me. It's nice that it removes more code.

@rdblue rdblue merged commit 1e43cb4 into apache:master Jun 12, 2022
@Fokko Fokko deleted the fd-use-a-singleton branch June 13, 2022 07:39
namrathamyske pushed a commit to namrathamyske/iceberg that referenced this pull request Jul 10, 2022
namrathamyske pushed a commit to namrathamyske/iceberg that referenced this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants