-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
refactor: use template strings, unify testing, remove dottie #10055
refactor: use template strings, unify testing, remove dottie #10055
Conversation
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.
Please keep dottie
dependency for now or atleast we will remove that in different PR. Dottie is used in some performance sensitive codebase. I want to measure the impace before we replace it with lodash get/set
Some tests for Postgres are failing |
Codecov Report
@@ Coverage Diff @@
## master #10055 +/- ##
==========================================
- Coverage 96.3% 96.3% -0.01%
==========================================
Files 63 63
Lines 9412 9410 -2
==========================================
- Hits 9064 9062 -2
Misses 348 348
Continue to review full report at Codecov.
|
Re dottie, I did a rudimentary benchmark with 10mil iterations.
Reason I removed it is that you use |
We use Dottie still got upper hand (as per your benchmark), I would like to measure impact at-least independently before deciding to remove it |
I don't think it will have any measureable impact in actual consumer use but I will revert the specific changes. Here is my benchmark:
|
Thanks @SimonSchick |
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
Last big PR for a while.
suite
andtest
within tests to use thedescribe
andit
functions.