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.
CopyableThreadContextElement implementation #3227
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
CopyableThreadContextElement implementation #3227
Changes from all commits
fe34623
091a3d7
1857404
774031e
8563d46
7757b06
027e267
05479c9
1f7fa0b
e9f31d9
be8161a
a85e8fb
f572e8b
ff5083b
31b57ad
db0d613
20bedf9
479481e
a32036c
64163c1
9750b59
5360968
20a0da8
e9f02f1
9dfb868
60c43c9
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.
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 believe the original version is more readable (and debuggable)
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.
You're right, for some reason I can't place a breakpoint on block subexpressions here. How about this then?
If not, feel free to just disregard 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.
In general, I prefer
over
to keep the number of nested blocks minimal. This is, of course, opinionated, so we can stick to the most "concise" one or to the proposed middle-ground if you do object
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.
In general, I'm the opposite and will typically prefer the other way because it highlights the symmetry between the cases and simplifies mental parsing of the control flow. In particular, in this case, I certainly don't find nesting to be an issue.
In any case, we're splitting hairs here. Both versions are perfectly fine, I think.