Skip to content
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

Fixed exception handling in TransmitAsync #321

Conversation

milopezc
Copy link
Contributor

Fixed exception handling in TransmitAsync.

@codecov-io
Copy link

codecov-io commented Aug 15, 2019

Codecov Report

Merging #321 into v2.1 will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             v2.1     #321      +/-   ##
==========================================
+ Coverage   89.42%   89.64%   +0.21%     
==========================================
  Files          31       31              
  Lines        1939     1951      +12     
  Branches      366      369       +3     
==========================================
+ Hits         1734     1749      +15     
+ Misses        157      156       -1     
+ Partials       48       46       -2
Impacted Files Coverage Δ
src/StreamJsonRpc/Resources.Designer.cs 68.96% <100%> (+0.54%) ⬆️
src/StreamJsonRpc/JsonMessageFormatter.cs 86.63% <100%> (+0.06%) ⬆️
src/StreamJsonRpc/JsonRpc.cs 93.35% <100%> (+0.23%) ⬆️
src/StreamJsonRpc/ProxyGeneration.cs 98.28% <0%> (+0.34%) ⬆️
src/StreamJsonRpc/MessageHandlerBase.cs 97.61% <0%> (+2.38%) ⬆️

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 cfd93ae...73f0c5d. Read the comment docs.

@AArnott
Copy link
Member

AArnott commented Aug 15, 2019

@milopezc, When I asked for you to rebase your changes to the v2.1 branch, it didn't require that you close your PR and open a new one. You can edit the target branch on an existing PR.
But more importantly, merely changing the target branch or opening a new PR with a different target branch is insufficient. You must rebase your commits to the v2.1 branch, which you evidently didn't do. Check the commits list in this PR and you'll see some commits unrelated to your change because they're still based on master.

You can fix this, without closing this PR, by following these steps on your machine:

git rebase --onto upstream/v2.1 8520f68~1 dev/milopezc/FixedTransmitExceptionHandling_959430 
git push -f

The commit ID that appears in that command line is your first commit related to this change. You're telling git to move your topic branch (last argument) to v2.1 (first argument), moving all commits up to and excluding the commit named. By stating your commit ID with ~1 at the end, that's git notation to identity the commit before it, so you're stating to consider the base of your topic branch to be just before your first commit, so that it moves your first commit.

That will take care of the rebasing. The next thing I would recommend is that you squash your commits since there are quite a few little fix-up commits. You can do that with these commands:

git merge upstream/v2.1
git reset --soft upstream/v2.1
git commit -m "Fixed exception handling in TransmitAsync"
git push -f

@milopezc milopezc force-pushed the dev/milopezc/FixedTransmitExceptionHandling_959430 branch from 34d30d0 to 7c2b07f Compare August 15, 2019 19:18
Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, once you get the build to pass.

@milopezc milopezc force-pushed the dev/milopezc/FixedTransmitExceptionHandling_959430 branch from dabffee to 73f0c5d Compare August 15, 2019 22:29
@AArnott AArnott merged commit 8f34c5a into microsoft:v2.1 Aug 15, 2019
@AArnott AArnott added this to the v2.1 milestone Aug 15, 2019
AArnott added a commit that referenced this pull request Jan 14, 2025
Revert "Merge pull request #319 from AArnott/betterTestingLibTemplate"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants