Conversation
|
Current output is: |
|
@erikzhang to disable some warning, just put the code (and message explaining comment) under "None" section. Step by step we can improve these rules. One interesting case is: I think it's right, we could define local variables starting with lower case letters. There are other possibilities too. |
|
This can be right too: This line is: Also agreee with Also Do you have a local code formatter @shargon? Could try to format them to "standard", to see which warnings still continue? |
|
We could remove and this more than 1 thousand in visual studio ... |
This have no sense |
|
@shargon Here I have "only" 200 warnings... if you have 1000, looks like the Perhaps removing the SolutionDir here? I can try to adjust, but it's working already for us (pure dotnet builders). It is important that it takes effect for you visual studio guys. |
|
Done |
|
Working fine for me Shargon, is it working locally now? |
neo.UnitTests/neo.UnitTests.csproj
Outdated
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <!-- <PackageReference Include="StyleCop.MSBuild" Version="6.1.0" /> --> |
|
@shargon let's vote on a list of what's necessary, and what's not. Some sampling here (list codes is ordered. Edit the comment here.): Anything you agree? Some I agree, ordering on includes for example is a good practice on many languages. Space-related stuff we can cut I think... and closing brackets stuff too. |
|
@shargon zero warnings now \o/ I disable most of the "crazy" rules, and also those that were "too strict". I kept many rules, mostly those related to variable naming, which is very important to us (start following a minimal standard). @erikzhang Please review the changes I did on the unit tests code, all of them were done following a standard (nothing from my head). If you think any change was not necessary, just mark it, and we try to find which rule that caused it (so we blacklist it). |
vncoelho
left a comment
There was a problem hiding this comment.
Amazing job, @igormcoelho and @shargon!
|
Is this only effective for unit testing? |
Nope, just warming up here.. next step is the whole project. |
|
I need to see how it will affect the whole project, then we can discuss which rule set to be enabled or disabled. |
|
Ok, it's reasonable. I'll put here the real numbers. |
|
|
A quick analysis:
The rest is upper-case vs lower-case situations... I think we should adjust to some standard Erik. Even if not perfect, it's easier to organize things. Example: |
|
Let's merge other important PRs first. |
Codecov Report
@@ Coverage Diff @@
## master #857 +/- ##
=======================================
Coverage 43.57% 43.57%
=======================================
Files 177 177
Lines 12547 12547
=======================================
Hits 5467 5467
Misses 7080 7080Continue to review full report at Codecov.
|
|
@igormcoelho please don't spam with huge outputs xD |
|
@erikzhang, maybe it is better to decide this here first, this PR changes many files and will easily become hard to solve the conflicts...aheuahueaea Is there anything bad in this styles or warning? |
|
I think Erik wants to do the whole project at once... when we have some "free" time, on other PR, we advance on this one... right now, conflicts are not impossible, but may become harder. Not very important too, we can re-open if necessary. |
|
Focusing on main PR. We can revive this in the future, if necessary. |
I think a great step for us is to adopt a linter and code formatting "cop" to help us maintain good quality code.
I tested StyleCop and it looks like a good alternative. It has many possibilities to configure it, according to project needs.
I just put it with some default template, but we can work on it (on tests folder) before putting it on the main project.
PS: we can give some claps to this guy here: https://medium.com/@michaelparkerdev/linting-c-in-2019-stylecop-sonar-resharper-and-roslyn-73e88af57ebd