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

[hxb] Reader: lazy type restoring #11873

Merged
merged 5 commits into from
Dec 13, 2024
Merged

[hxb] Reader: lazy type restoring #11873

merged 5 commits into from
Dec 13, 2024

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Dec 11, 2024

Extracted from #11866

@Simn
Copy link
Member

Simn commented Dec 11, 2024

One concern might be that the lazies are not resolved before the end of typing. While they should eventually be handled by some follow, this might occur at some point during filtering or even generation and it's not very clear what exactly that means then.

In the real typer we use this make_lazy function which has a force parameter that governs if the lazy is explicitly resolved in a delay. Perhaps it would be prudent to have something similar in the hxb reader, which shouldn't be too hard to add via the api.

@skial skial mentioned this pull request Dec 11, 2024
1 task
@kLabz
Copy link
Contributor Author

kLabz commented Dec 11, 2024

We surely don't want it to be resolved unless really needed during display requests, so that would need to be handled only for full typing (which would bring back some kind of should_lazy_wrap)

@Simn
Copy link
Member

Simn commented Dec 11, 2024

Yes I'm talking about non-display requests, that's why I mention filtering and generators.

What I don't like about should_lazy_wrap is that we're putting more logic into the reader. If we keep adding more modes and states then it's going to be quite difficult to reason about anything. So instead of having the reader trying to make informed decisions about whether or not to lazify anything, the API provided to the reader should make these decisions.

@kLabz kLabz merged commit c878315 into development Dec 13, 2024
99 checks passed
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.

2 participants