-
Notifications
You must be signed in to change notification settings - Fork 471
Fix result examples #7914
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
Fix result examples #7914
Conversation
tests/docstring_tests/DocTest.res
Outdated
// Let's add the examples inside a Test module because some examples | ||
// have type definitions that are not supported inside a block. | ||
// Also add unit type `()` | ||
// Also add unit type `()`. Also wrap in async to make top-level awaits work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be controversial, but all tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is no noticeable performance difference from this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope:
master
node scripts/test.js -docstrings 8.49s user 2.25s system 365% cpu 2.938 total
fix-result-examples
node scripts/test.js -docstrings 8.44s user 2.04s system 357% cpu 2.933 total
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
1fa0790
to
8f904fd
Compare
tests/docstring_tests/DocTest.res
Outdated
// Let's add the examples inside a Test module because some examples | ||
// have type definitions that are not supported inside a block. | ||
// Also add unit type `()` | ||
// Also add unit type `()`. Also wrap in async to make top-level awaits work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is no noticeable performance difference from this?
I'm fine with this as-is, can't imagine the perf overhead would be something we need to care about. But if we wanted to care, a quick optimization could be to only wrap if the generated JS text contains |
Vibecoded examples of course, sorry!