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

dotnet format #1054

Merged
merged 2 commits into from Aug 24, 2019

Conversation

@igormcoelho
Copy link
Contributor

commented Aug 24, 2019

cosmetic change, related to base formatting issue: #982

@igormcoelho igormcoelho requested a review from vncoelho Aug 24, 2019

@vncoelho
Copy link
Member

left a comment

Very good, @igormcoelho.

@shargon, this may solve what we were discussing in #1053.
Otherwise, it would be critical to have a PR with name "fix" changing the history of the "Consensus", it could be misunderstood.

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

@erikzhang, I think that we need a guidelines for formatting, this is causing some "critical" problems to us, in which normal PR are changing lines and providing a misundertood interpretation both to us and also to others that follow the repo.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 24, 2019

Codecov Report

Merging #1054 into master will not change coverage.
The diff coverage is 92.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1054   +/-   ##
=======================================
  Coverage   61.33%   61.33%           
=======================================
  Files         196      196           
  Lines       13406    13406           
=======================================
  Hits         8223     8223           
  Misses       5183     5183
Impacted Files Coverage Δ
neo/Network/RPC/Models/RpcResponse.cs 65.9% <ø> (ø) ⬆️
neo/Cryptography/ECC/ECDsa.cs 96.05% <ø> (ø) ⬆️
neo/SmartContract/StorageContext.cs 0% <ø> (ø) ⬆️
neo/Consensus/PrepareResponse.cs 100% <ø> (ø) ⬆️
.../Consensus/RecoveryMessage.CommitPayloadCompact.cs 46.15% <ø> (ø) ⬆️
neo/Persistence/Helper.cs 7.89% <ø> (ø) ⬆️
neo/Wallets/WalletAccount.cs 100% <ø> (ø) ⬆️
neo/UIntBase.cs 54.54% <ø> (ø) ⬆️
...sensus/RecoveryMessage.ChangeViewPayloadCompact.cs 46.15% <ø> (ø) ⬆️
neo/SmartContract/Manifest/ContractAbi.cs 100% <ø> (ø) ⬆️
... and 178 more

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 8960dd8...5b18397. Read the comment docs.

@shargon shargon self-requested a review Aug 24, 2019

@lock9
lock9 approved these changes Aug 24, 2019

@lock9 lock9 merged commit 34faf4e into neo-project:master Aug 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shargon

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

#1016 (comment)

What did we discuss about waiting for review?

222 files changed merged y 3 hours ...

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

@lock9, "With Great Power Comes Great Responsibility"...aehuahuea
I think it would have been wiser to wait...aehuaheua

Anyway, let's wait for more comments on this to see if we need to proceed with any adjustment.

@vncoelho vncoelho deleted the igormcoelho:dotnet-format branch Aug 24, 2019

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

@igormcoelho, can you provide us with the guidelines of commands that you used for that?

I think this check can be in Travis as well!

@lock9

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

Sorry, I didn't notice the PR was created 5 hours ago, I thought it was 5 days old, sorry.
In any case, it does not change any logic, any chance this can cause problems? Again, I'm sorry, my bad!

lock9 added a commit that referenced this pull request Aug 24, 2019
Revert "dotnet format (#1054)"
This reverts commit 34faf4e.
@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

best thing is lock on a github bot.

@erikzhang

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

What the hell was this??

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

Sorry @erikzhang, it was necessary to finally fix all mixed crlf file encodings 😂 It was breaking diffs everywhere. Now it should be fine 👍

We should discuss which bom prefix do we want... it was all mixed, so I removed. But perhaps we prefer utf8, one good for your file editor (this is perhaps best for everyone).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.