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

[BUG] Documentation states you can exclude by attribute with short name or full name - cannot by long name #1589

Closed
tonyhallett opened this issue Jan 17, 2024 · 5 comments · Fixed by #1603
Assignees
Labels
enhancement General enhancement request

Comments

@tonyhallett
Copy link
Contributor

Describe the bug
From https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/GlobalTool.md

You can also ignore additional attributes by using the ExcludeByAttribute property (short name or full name supported):

To Reproduce
Create a custom attribute, apply it at any level.
Supply fully qualified through one of the drivers

Expected behavior
Is it excluded

Actual behavior
Is not excluded

Additional context

private bool IsExcludeAttribute(CustomAttribute customAttribute)

None of the Instrumenter tests supply the attributesToIgnore as fully qualified
e.g

InstrumenterTest instrumenterTest = CreateInstrumentor(attributesToIgnore: new string[] { excludedAttribute });

@daveMueller
Copy link
Collaborator

daveMueller commented Jan 20, 2024

OK I see the problem. From my point of view we should adapt the documentation and/or consider implementing also the fully qualified attribute name. I don't have any details about why it was decided in the past for coverlet to only check the attribute name. Maybe forgotten to consider or the one implementing this didn't see the need for it. But from my point of view, it wouldn't be much implementation effort to additionally check for the fully qualified name.

I just wonder about the use cases a bit? The only thing I can think of is someone having two attributes with the same name in different namespaces of his application and wants to exclude both of them.

@MarcoRossignoli @Bertk What do you think? Should we implement this? Even if it is just one use case I can think of, it seems valid to me.

@daveMueller daveMueller added feature-request New feature request enhancement General enhancement request and removed untriaged To be investigated feature-request New feature request labels Jan 20, 2024
@tonyhallett
Copy link
Contributor Author

tonyhallett commented Jan 20, 2024

Changing the documentation is probably sufficient. Having two similarly named attribute types serving different purposes seems highly unlikely.
Bringing it to your attention as it is a bug given the documentation. Fine Code Coverage has been following the documentation all this time. I expect that the only time you would encounter this bug is with .Net Framework where you cannot apply ExcludeFromCodeCoverage at the assembly level. You create and apply your own exclusion attribute and decide to qualify in coverlet.

@Bertk
Copy link
Collaborator

Bertk commented Jan 21, 2024

@daveMueller I think full name shall be supported.

@MarcoRossignoli
Copy link
Collaborator

I don't recall but maybe we did some regression here, we could support full name too.

@daveMueller
Copy link
Collaborator

OK I work on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants