Gh 66 substituting for internal members

[tpodolak]

Closes #66

[coveralls]

Pull Request Test Coverage Report for Build uaxvm5k358vombb5

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.05%) to 96.522%

---

Files with Coverage Reduction	New Missed Lines	%
../src/NSubstitute.Analyzers.Shared/Extensions/ISymbolExtensions.cs	1	91.67%

Totals
Change from base Build 4chbhptu0yabbt2i:	0.05%
Covered Lines:	1110
Relevant Lines:	1150

---

💛 - Coveralls

[tpodolak]

Hi @dtchepak as discussed in #66 detecting internal member substitution is done as one diagnostic. Additionally, I've changed the category name to "Non-substitutable member" and also adjusted relevant items in the documentation. As far as I can tell (checked on VS2017), changing category name doesn't influence the suppression, so if someone used suppression old category name

[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Non virtual substitution", "NS1002:Non-virtual setup specification.", Justification = "Reviewed")]

this will still work, even though we recommend now using the new category name. If you have time please double-check that, because if I am wrong this will be a breaking change.
PS
This PR comes with diagnostics only, we might think about adding some fix provides later on (to keep this PR relatively small) #70

[dtchepak]

@tpodolak As far as I can tell from the documentation, the category name will only be used if there are conflicts with other rule identifiers?

In my test project I can replace the category with "asdfasdf", "", or null and it will still suppress the error if I have the correct NSxxxx code there. So I think this would only be a breaking change if there is something else producing NSxxxx errors.

If I'm correct about the category only being used in that case, next time we update categories maybe we should just make them all NSubstitute.Analyzers to differentiate from anything else that produces NSxxxx errors?

[tpodolak]

If I'm correct about the category only being used in that case, next time we update categories maybe we should just make them all NSubstitute.Analyzers to differentiate from anything else that produces NSxxxx errors?

@dtchepak I am not sure if I understand you correctly, are you suggesting to rename all categories to NSubstitute.Analyzers, or to prefix them all with NSubstitute.Analyzers?

[dtchepak]

@tpodolak I'm not sure if it should be a prefix or the full category. I don't really understand what the category is for. We can't suppress based on category? From the MS docs it looks like it may just be used to differentiate conflicting codes?

I've seen categories like Microsoft.Design and Microsoft.Maintainability, Microsoft.Design. Maybe we should have things like NSubstitute.Usage, NSubstitute.ArgumentSpecification etc?

I really don't know about any of this, I just thought I'd ask the question as I noticed that the category listed in the suppression doesn't seem to make any difference at all, which makes me wonder if it is of any use at all! 😂

[tpodolak]

For me, the categories are just logical grouping of similar diagnostics. I would say there are targeting the end users rather anything else. I don't think we can suppress by category so it looks like they are only used in case of conflicts. NSubstitute.Usage, NSubstitute.ArgumentSpecification sounds good for me, definitely better than the current one. Will add an enhancement for that later on. However, I would rather keep the categories so I wouldn't join them all into one

[dtchepak]

Will add an enhancement for that later on. However, I would rather keep the categories so I wouldn't join them all into one

Sounds good 👍