Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

refactoring of PrettyPrinter: extract base class BasePrinter #571

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

h0nzZik
Copy link
Contributor

@h0nzZik h0nzZik commented Aug 2, 2023

The first point of issue #473 says:

Factor out a base class like BasePrinter which only holds a symbol table and does unparsing. This must be done in a way that allows the PrettyPrinter class to: (i) only compute the symbol table a single time when calling it (memoize it), and (ii) only compute the symbol table if it's actually needed (compute it lazily).

Is something like this what you meant, @ehildenb ?

@h0nzZik h0nzZik marked this pull request as ready for review August 2, 2023 13:58
@h0nzZik h0nzZik requested a review from ehildenb August 2, 2023 13:58
@ehildenb ehildenb requested a review from tothtamas28 August 2, 2023 17:09
@ehildenb
Copy link
Member

ehildenb commented Aug 2, 2023

@tothtamas28 was the one that suggested this architecture, so we should ask him if it meets the requirements.

@ehildenb
Copy link
Member

ehildenb commented Aug 2, 2023

I personally think that things have been working fine without friction so far, so maybe this change is not needed. But if it improves something, without degrading performance, then that's good.

@tothtamas28
Copy link
Collaborator

tothtamas28 commented Aug 3, 2023

@tothtamas28 was the one that suggested this architecture, so we should ask him if it meets the requirements.

What I had in mind with BasePrinter was just to have straightforward unparsing based on an (optional) SymbolTable:

class BasePrinter:
    _symbol_table: SymbolTable | None

    def __init__(self, symbol_table: SymbolTable | None = None):
        self._symbol_table = symbol_table

    def print(self, kast: KAst) -> str:
        # do simple dispatch to protected methods
        ...

    def _print_kas(self, kas: KAs) -> str:
        ...

    ...

This must be done in a way that allows the PrettyPrinter class to: (i) only compute the symbol table a single time when calling it (memoize it), and (ii) only compute the symbol table if it's actually needed (compute it lazily)

Optional lazyness can be built into BasePrinter (take a SymbolTable | Callable[[], SymbolTable] | None), but this is not necessary to enable PrettyPrinter to be lazy with the symbol table:

class PrettyPrinter:
    def __init__(self, ...):
        super().__init__()  # lazily initialize that attribute

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants