Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: some optimize #622
feat: some optimize #622
Changes from 13 commits
fa775ee
ba1b77f
80b43ec
c671bd1
cd7a67c
a04d408
eb4ca10
c476440
94c7b45
116db2d
7dfe6db
a82d4ac
071e83a
422a8b4
55325f0
14a8bab
8835015
839d37f
7f83a71
eda3634
830e51a
d4f2274
43d37f3
9143806
14c2f31
bf97aec
f5f0af1
629010c
1e66167
bc59486
cce4247
70a2f36
c412ee9
b47dbfa
021515c
ca34907
4eb0170
c506389
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Em, if we can‘t check the result, what's the meaning of it?
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.
Move the test cases previously used for concurrency testing to the benchmark.
And although the results cannot be viewed directly, CI will automatically compare them with previous results.
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.
Do you mean 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.
Yes, currently no data.
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.
If we are returning an error, we don't need to return the manager because chaining won't make sense to the user anyway. (Sorry for spamming here 😅;)
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.
makes sense.
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.
And I think
BuildSession
can directly pass driver name instead of handler?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.
Hmm, we can pass the driver name directly but need to return an error as the second argument. Also, I believe we don't need the Driver method anymore if we pass driver name.
cc: @hwbrzzl
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 is a bug fix.