Skip to content

One approach to fixing lazy parsing of registries via from-ast #1185

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

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

Conversation

frenchy64
Copy link
Collaborator

@frenchy64 frenchy64 commented Apr 10, 2025

The call to -property-registry was defeating all the laziness of -delayed-registry in from-ast.

I'm not sure which way to go with this. Some options:

  1. Remove call to -property-registry and allow :properties to be a map from key to IntoSchema. This works for straightforward cases because -property-registry is called again to coerce the properties back to forms, and -property-registry always calls m/schema first. But this changes the output of (-> s m/properties :registry).
  2. Add more indirection in the problematic call to -property-registry to wrap schemas in a similar way to -proxy-schema, such as (-property-registry r options #(reify Schema (-validator [_] (m/schema % options)) ...)
  3. Remove the use of -delayed-registry. Performance is apparently still great without laziness compared to vector syntax.

This PR implements option 1 to demonstrate the problem.

Also refactors cond into if, removing redundant tests.

@frenchy64 frenchy64 changed the title WIP: fix lazy parsing of registries via from-ast One approach to fixing lazy parsing of registries via from-ast Apr 11, 2025
@frenchy64 frenchy64 added bug Something isn't working question Further information is requested labels Apr 11, 2025
@frenchy64 frenchy64 marked this pull request as ready for review April 11, 2025 00:05
@opqdonut
Copy link
Member

opqdonut commented May 7, 2025

I tried to understand this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants