-
Notifications
You must be signed in to change notification settings - Fork 154
Drop the benchmark dependency
#2407
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR. Can you fix typechecking errors? |
065a67c to
38e669c
Compare
|
Oops, yeah |
|
We have addressed a few CI issues this week, which should make this PR's build green. Can you rebase the PR? Thx |
38e669c to
1b0f472
Compare
It is trivial, don't have to take on a dependency for that.
1b0f472 to
947c6f8
Compare
|
Based on CI failures, it looks like some dev dependencies Tapioca relies on ( |
Morriar
left a comment
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.
Code Review - Ruby Reviewer
Found 2 issue(s) that may need attention:
1. [MEDIUM] Missing Sorbet Signatures
Found 1 new methods but only 0 Sorbet signatures. Type signatures improve code safety and documentation.
Suggested fix:
Add Sorbet sig annotations to new methods.
Example:
sig { params(name: String).returns(User) }
def find_user(name)
# ...
end2. [MEDIUM] No Test Changes
Found 2 code file changes but no test file changes. New functionality should include tests.
Suggested fix:
Add tests for the new or modified functionality.
This is a draft review. Please review manually before approving.
Motivation
tapioca currently depends on the
benchmarkgem because of a change to Ruby bundled gems. The functionality is trivial though, and I don't think you have to take on a dependency for that.Implementation
Just use
Process.clock_gettimedirectly.Tests
No behaviour change. I just ran some cli commands and they still print the time in seconds. Probably the output is also already asserted against somewhere.