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

Remove static Coverage object #393

Closed

Conversation

MarcoRossignoli
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli commented May 2, 2019

contributes to #387
closes #364
replace #366

  • Removed static InstrumentationTask.Coverage and used [Output] msbuild parameter

Instrumenter state is serialized using IInstrumentStateSerializer for now implemented with JsonSerializer

  • Organized coverlet internals in two main interface IInstrumenter and ICoverageCalculator
  • Organized cs files under project folder Instrumentation and Coverage
  • Cleanup msbuild Task parameter using auto prop

/cc @tonerdo @petli @sharwell


public override string ToString()
{
return JsonConvert.SerializeObject(this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for simplicity I used Json serializer.

public uint Ordinal;
}

public class BranchKeyConverter : TypeConverter
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need custom converter because Json serializer does not support serialization of complex dictionary key

@MarcoRossignoli MarcoRossignoli requested review from tonerdo and petli May 2, 2019 19:04
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #393 into master will decrease coverage by 0.69%.
The diff coverage is 95.33%.

@@            Coverage Diff            @@
##           master     #393     +/-   ##
=========================================
- Coverage   86.96%   86.26%   -0.7%     
=========================================
  Files          16       17      +1     
  Lines        2178     2220     +42     
=========================================
+ Hits         1894     1915     +21     
- Misses        284      305     +21

@MarcoRossignoli
Copy link
Collaborator Author

@tonerdo fixed conflicts...the change touch a lot of file if you can review this before others we avoid too much conflicts...I think(hope) this is the only "huge" update of checklist.

@tonerdo
Copy link
Collaborator

tonerdo commented May 12, 2019

@MarcoRossignoli this is a good start. I'm a bit skeptical about the drastic changes and I wonder if things could be simpler. The most important things that need to be serialized in the Coverage class is identifier, _module, _mergeWith, _useSourceLink, and _results (maybe _logger). I wonder if we can just extract those into their own class, serialize those and add a sort of "builder" method on Coverage that takes that class as a parameter.

Also, it's best not to do major style refactorings for important features so it doesn't get so large

@MarcoRossignoli
Copy link
Collaborator Author

Replaced by #409 keep this PR as useful notes.

@MarcoRossignoli MarcoRossignoli deleted the removestatics branch May 13, 2019 09:24
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.

MSBuild data should not be passed between tasks via static fields
3 participants