Closed fineol closed 2 years ago
The AppVeyor tests failed only due to the known #680 bug.
Merging #691 (b1499e3) into master (859190a) will increase coverage by
0.46%
. The diff coverage is83.33%
.
@@ 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
Impacted Files | Coverage Δ | |
---|---|---|
tds.go | 64.94% <50.00%> (-0.37%) |
:arrow_down: |
bulkcopy.go | 57.20% <57.14%> (ø) |
|
token.go | 61.85% <85.71%> (+0.43%) |
:arrow_up: |
mssql.go | 91.77% <91.17%> (+0.08%) |
:arrow_up: |
log.go | 100.00% <100.00%> (ø) |
|
net.go | 67.81% <0.00%> (+25.28%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 859190a...b1499e3. Read the comment docs.
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 - git rebase
or git pull
?
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 git pull
is typical which makes me want to ask the MSAL guys why they recommend using rebase on forks of their stuff https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/dev/CONTRIBUTING.md
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.