-
-
Notifications
You must be signed in to change notification settings - Fork 583
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
Extend dynamicRef keyword #886
Conversation
Codecov Report
@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 96.90% 97.05% +0.15%
==========================================
Files 20 20
Lines 3259 3290 +31
Branches 446 457 +11
==========================================
+ Hits 3158 3193 +35
+ Misses 79 77 -2
+ Partials 22 20 -2
Continue to review full report at Codecov.
|
@nezhar can you have a look at resolving the conflicts here now that the performance issues with the implementation have been addressed? |
ab759d5
to
2bec8b4
Compare
Thanks @nezhar. I'm about ready to merge this (sorry for the delay), but to be extra safe I'd like to re-run the benchmarks that @Stranger6667 optimized from #853 -- can you rerun the two they left in that issue there just to be sure we don't regress on them by merging this? |
FWIW, some of the work done during ref resolving is only relevant for newer drafts (after Draft 7), so maybe as a follow-up from this PR it would be nice to explore adding some conditional logic to avoid unnecessary work (e.g. the |
@Julian also sorry for the delay 🙂. I made another rebase and run the test from the linked issue. This are my results:
|
No worries! Will merge I think. Thanks! If you manage to figure out the remaining 2 skipped tests in draft2020 a PR would of course would be very much appreciated. So long as they still fail, my suspicion is that the implementation is still not 100% correct (even in the parts that pass), but it's definitely an improvement. Thanks again! |
The description in https://stackoverflow.com/a/69774520/5382179 helped me better understand how the keyword should actually work and the PR extends the missing parts of the current implementation of the keyword.
There might be a better way to implement this, but it would require bigger changes in the way the scopes and the validator functions are handled.