Skip to content

Conversation

@phaniva
Copy link

@phaniva phaniva commented Nov 27, 2020

Please review this design to see if it addresses the requirement. I changed it so that we can enable relative directory option for all report formats but only changed it for lcov for now.

@dnfadmin
Copy link

dnfadmin commented Nov 27, 2020

CLA assistant check
All CLA requirements met.

@MarcoRossignoli
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MarcoRossignoli MarcoRossignoli added the tenet-reporters Issue related to coverage output files(reporters) label Nov 27, 2020
@MarcoRossignoli
Copy link
Collaborator

@phaniva please sign the CLA.

I'll take a look asap

@phaniva
Copy link
Author

phaniva commented Dec 3, 2020

@MarcoRossignoli I have signed the CLA. Not sure if you received a notification automatically

@MarcoRossignoli
Copy link
Collaborator

@phaniva yep I see, bit busy this period, I'll take a look asap.

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

I like the idea, we can simplify and simply add a base class with this two helpers.

Sorry for the delay.


namespace Coverlet.Core.Helpers
{
internal class FilePathHelper : IFilePathHelper
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that extract a service is too much for a path helper...I mean we won't inject different implementation there are no different way to get that path from file system.
Should be enough add a new base class abstract ReporterBase and add this two method as protected and inherit the reporter that needs this logic.

Copy link
Author

Choose a reason for hiding this comment

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

So only the reporter classes that need this behavior will inherit from ReporterBase and rest of them will continue to implement IReporter?
Why not inherit every reporter class from ReporterBase to keep things consistent?

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli Mar 8, 2021

Choose a reason for hiding this comment

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

Ok I won't oppose, the important things is that we have less code as possible to maintain, let's go for base class inherited by reporters

Copy link
Author

@phaniva phaniva Mar 9, 2021

Choose a reason for hiding this comment

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

Cool. I am going to close this pr and open a new one with base class design.
Here is the new pr.
#1120

@phaniva phaniva closed this Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tenet-reporters Issue related to coverage output files(reporters)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants