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
Context Logger #691
Context Logger #691
Conversation
The AppVeyor tests failed only due to the known #680 bug. |
Codecov Report
@@ Coverage Diff @@
## master #691 +/- ##
==========================================
+ Coverage 70.56% 71.02% +0.46%
==========================================
Files 24 24
Lines 5188 5198 +10
==========================================
+ Hits 3661 3692 +31
+ Misses 1300 1280 -20
+ Partials 227 226 -1
Continue to review full report at Codecov.
|
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.
oh the merge to come for #690 :-)
Oh my! I'm glad to assist if you want. |
@fineol Well it'd be great to have a review on that PR for starters. I've not seen activity on any PR the past couple weeks. If your change goes before mine, what kind of merge is typically more effective - |
I just reviewed #695 and then saw your comment here. I'll try to take a look at #690 too. In general, rebasing is frowned upon for commits that are exposed for others to reference. Rebasing rewrites the commits, which can wreak havoc on anyone who has referenced the original commits. I personally don't rebase anything that I have pushed to a remote repository to which anyone else has access, even my direct colleagues. In this case, because the general public can see (and use) the commits in our PRs, I think the answer is clear: git pull + git merge rather than rebase. |
yeah I thought thx |
Rebasing makes for a (potentially much) neater looking commit history, which is nice. They probably want that neatness at the start of the pull request. Also, at least in the past, there were some review tools that didn't handle synchronization by merging very well. They made it look like the changes merged in from main were part of the changes in the pull request, which was confusing and added a lot of extra work to reviewing pull requests. But I think these days most review tools are better about that and "hide" the changes merged from main so that you only review the new changes made specifically for the pull request. I'll also note that their instructions refer to what to do before issuing a pull request. Before you issue the pull request, the changes can be considered semi-private, and the consequences of rewriting those commits are not so severe. But after you issue the pull request, things change. For example, if someone reviewing the pull requests makes a comment about a specific commit and you subsequently rewrite that commit by rebasing, the comment won't make sense anymore (and may be lost altogether)? |
This pull request resolves #680 and adds a ContextLogger interface to improve the capabilities of the existing Logger interface. Compared to the Logger interface, ContextLogger:
This pull request maintains the existing Logger for backwards compatibility. The developer can choose to use either interface, but not both at the same time. The last one set wins.