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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix crlf on neo.UnitTests and set all files to not use BOM (byte order mark) #1057

Merged
merged 43 commits into from Aug 27, 2019

Conversation

@igormcoelho
Copy link
Contributor

commented Aug 25, 2019

Change related to base formatting issue: #982

Now reformatting all unit tests... sorry @erikzhang 馃槀

shargon and others added 12 commits Aug 25, 2019
@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

We can discuss here a little bit before merging... this is what is being done:

  • fixing all crlf
  • removing random bom prefix to no_bom

We need to double check if chinese comments are still working on files. If something strange happens, we can enforce an UTF-8 bom prefix on all files.

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Maybe UTF-8 would be better. What do you think, @erikzhang?

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

UTF8 is better

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

Ok, situation is like this (totally non-standard):

       0     190       0  UTF-8     text    ./UT_BigDecimal.cs
       0     110       0  no_bom    text    ./Ledger/UT_StorageItem.cs
       0     147       0  UTF-8     text    ./Ledger/UT_PoolItem.cs
       0     130       0  no_bom    text    ./Ledger/UT_StorageKey.cs
       0     257       0  UTF-8     text    ./Ledger/UT_MemoryPool.cs

on neo

       0      14       0  UTF-8     text    ./obj/neo.csproj.nuget.g.props
       0     287       0  no_bom    text    ./Helper.cs
       0     154       0  no_bom    text    ./UInt256.cs

I'll unify all as UTF-8, not only neo unit tests

@igormcoelho igormcoelho changed the title fix crlf on unit tests fix crlf on neo.UnitTests and set all BOM to UTF-8 (neo/ and neo.UnitTests/) Aug 25, 2019

@igormcoelho igormcoelho requested review from shargon, vncoelho and erikzhang Aug 25, 2019

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

Everything is UTF-8 BOM now.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 25, 2019

Codecov Report

Merging #1057 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1057   +/-   ##
======================================
  Coverage    61.3%   61.3%           
======================================
  Files         196     196           
  Lines       13412   13412           
======================================
  Hits         8222    8222           
  Misses       5190    5190
Impacted Files Coverage 螖
neo/IO/Caching/ReflectionCacheAttribute.cs 100% <酶> (酶) 猬嗭笍
neo/SmartContract/ExecutionContextState.cs 100% <酶> (酶) 猬嗭笍
neo/IO/Caching/ReflectionCache.cs 92.85% <酶> (酶) 猬嗭笍

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 98173cf...ad7bac1. Read the comment docs.

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Problem was my git config --global core.autocrlf set to true.
We updated description accordingly.

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

dotnet/format#347 (comment)

We need the version 4.0

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

I believe it is better to come back with the original verification with dos2unix. Otherwise we are going to need to change a bunch of lines.

If we need to change the lines is because is needed, and these lines are wrong

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@shargon I had to disable crlf option on dotnet

This is the most important one!

@igormcoelho Please, try again with the version 4.0
dotnet tool install -g dotnet-format --version 4.0.40103 --add-source https://dotnet.myget.org/F/format/api/v3/index.json

Is better to use only one tool for this purpose

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

looks like these options only work for version 3.1.37+. How will we inform this to users?

The user only need to know the rules, this is a check for ensure that they fit these rules

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Besides the version 3.1.37+, now I discovered @shargon! It was opposite, that's why it was strange... the formatter was enforcing crlf, while we are trying to remove crlf and replace by lf. It's fixed now 馃憤

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

oh man, just crashed my commit hahaha same file... same line,,, 馃槀

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

But do you changed the file for lf?

igormcoelho and others added 2 commits Aug 26, 2019
@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Yes, I was removing dos2unix commands. Will try again :P

Again kkkkk

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Just need to remove dos2unix now and it's finished.

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

image

I think that is working well now

.editorconfig Show resolved Hide resolved
@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

#1057 (comment)

I think that we should decide the tab style, more changes yes, but is only for today

@shargon
Copy link
Member

left a comment

I would like that we take a decision about the intend_style variable too, but is ok for me (tab is better for me). Also, @igormcoelho do you think that is better LF than CRLF? why? Anyway, now is good to me.

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

I tried again the tab, it's unfeasible now, really... it will change everything from scratch, we need to discuss how to do this properly, at this scale.
Regarding lf vs crlf, I think lf is better because it's the standard ("\n") for many algorithms, including diff... that's why things break when mixing crlf ("\r\n").
Historically, windows IDEs have no problem with lf, and they even create files like that, but the opposite is more complicated to handle... the code is 99% lf already, so changing to crlf is also a huge change (and will often break on linux servers and editors).

Better to move on now.

@vncoelho
Copy link
Member

left a comment

Worked again.

@shargon

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Is ready to merge for me when travis finished

@shargon shargon merged commit 5995f32 into neo-project:master Aug 27, 2019

1 check passed

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

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

I don't think we can merge this kind of thing so hurriedly.

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

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

I agree, @erikzhang, this was really special case I think, because it was becoming hard to open other PRs without merging this one.

Anyway, fell free to adjust anything you think is needed.

@erikzhang

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

We should remove the TODO.

@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

That is true, @erikzhang.
#1067

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