Logging context convenience functions for coroutines #65
Conversation
rocketraman
force-pushed
the
rg/context-convenience-fns
branch
from
d654f45 to
fe7b022
Compare
There was a problem hiding this comment.
I am not able to find key override tests in a nested setting, that is (in pseudocode):
withLoggingContext(map = (k1->k1v, k2->k2v), stack = [s1v, s2v]) {
// assert context initialization
assert(map, (k1->k1v, k2->k2v));
assert(stack, [s1v, s2v]);
// override context
withLoggingContext(map = (k3->k3v, k4->k4v), stack = [s3v, s4v]) {
// assert context initialization
assert(map, (k3->k3v, k4->k4v));
assert(stack, [s3v, s4v]);
// override context
withAdditionalLoggingContext(map = (k4->k4v*, k5->k5v), stack = [s5v]) {
assert(map, (k3->k3v, k4->k4v*, k5->k5v));
assert(stack, [s3v, s4v, s5v]);
}
// assert context restoration
assert(map, (k3->k3v, k4->k4v));
assert(stack, [s3v, s4v]);
}
// assert context restoration
assert(map, (k1->k1v, k2->k2v));
assert(stack, [s1v, s2v]);
}
Unless they are already there, could you add these too, please?
There was a problem hiding this comment.
@rocketraman, I am looking at changes in 713f1c4, and I am not seeing any key override. That is, there is nowhere you override an existing key. You always add new keys and verify them. Am I missing something?
There was a problem hiding this comment.
Right, I misinterpreted your original comment. Added a new test for key override and restoration.
|
@rocketraman, the CI is failing, could you also fix the build too, please? |
rocketraman
force-pushed
the
rg/context-convenience-fns
branch
from
fe7b022 to
713f1c4
Compare
|
@vy Have made the updates you suggested, please re-review. Regarding the build, I'm not sure what is going on there. Seems to be the bnd plugin reporting that a major semver update is required. The changes I've made shouldn't violate that requirement -- no existing signatures were changed or removed. Looking at the baseline report, it seems to think that the hash differences in some resources are violating the major semver requirement, but that seems silly: Perhaps we use https://github.com/bndtools/bnd/tree/master/maven-plugins/bnd-baseline-maven-plugin#diffignores? |
|
You need to look at the end of the BND report: This change requires a minor version bump to |
@ppkarwasz Thank you, my brain was confused between MAJOR/MINOR so I was looking at the wrong place in the report. Have pushed a commit to bump to |
rocketraman
force-pushed
the
rg/context-convenience-fns
branch
from
0270d05 to
019ac54
Compare
src/changelog/1.5.0/add-coroutine-context-convenience-functions.xml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@rocketraman, I am looking at changes in 713f1c4, and I am not seeing any key override. That is, there is nowhere you override an existing key. You always add new keys and verify them. Am I missing something?
rocketraman
force-pushed
the
rg/context-convenience-fns
branch
from
019ac54 to
3d21dcb
Compare
|
@rocketraman, I would appreciate it if you don't force-push. This renders all historical conversations detached, since associated commits don't exist anymore. |
There was a problem hiding this comment.
@rocketraman, LGTM. 👍 Thanks for your patience and prompt reaction during the review process. 🙇 You can merge the PR at your convenience.
@vy Yeah, GitHub is quite poor at managing code reviews with force pushes, unfortunately. My preference is generally to sacrifice the GitHub PR cleanliness in favor of a clean commit history, as the former is transient and the latter is forever. However, I recognize this is personal preference and most people tend to prefer keeping the PR clean and not caring too much about the commit log. Its unfortunate that GitHub isn't better at this, given that even git.git upstream prefers the force push / clean commit history approach. |
@rocketraman, we (almost) always use |
That is really a question of commit style. Personally I prefer force pushes like @rocketraman and I almost never make merge commits. Unless the number of commits is high or commits have no well-defined purposes (e.g. a contributor changes his contribution back and forth) I don't squash either. |
We were already able to create logging context for coroutines, but it was more verbose than it should be (#64). Create some convenience functions to make it easy to set (or add to) the logging context for a coroutine.
Resolves #64.