Moving pinning attributes and the pinned analyzer #921
Conversation
alamarre
requested review from
mthjones,
a team and
cesar-d2l
and removed request for
a team
|
|
||
| m_analyzers = analyzers.ToImmutableArray(); | ||
|
|
||
| AdditionalText allowedList = new StringAdditionalText( "UnpinnedAllowedList.txt", $@"System.Collections.Generic.Dictionary`2" ); |
There was a problem hiding this comment.
The other analyzers use it. I removed the other methods that aren't used, but left minor changes like this so that it was less work to do it as 3 PRs without missing anything, since some of the changes came from working on the must be pinned analyzer, but were for the recursive pinning.
| using Microsoft.CodeAnalysis; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace D2L.CodeStyle.Analyzers.Tests.Pinning { |
There was a problem hiding this comment.
Could these be done as spec tests instead? See the "spec" folder in this project (spec = specification 🤷 )
There was a problem hiding this comment.
Is there documentation on them?
There was a problem hiding this comment.
With https://github.com/Brightspace/D2L.CodeStyle/blob/main/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs as an example
- Add a first-line comment specifying the analyzer to load: https://github.com/Brightspace/D2L.CodeStyle/blob/main/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs#L1
- Put a multi-line comment around the syntax on which you're expecting a diagnostic,
/* DiagnosticName([messageArg[, ...]]) */ syntax /**/: https://github.com/Brightspace/D2L.CodeStyle/blob/main/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs#L94 - Multiple diagnostics on the same syntax are separated by pipe characters:
There was a problem hiding this comment.
I've moved the analyzer to it's own branch and implemented spec tests in there. You can preview them if you want. main...alamarre/add-pinned-analyzer
…on recent discussion
alamarre
force-pushed
the
alamarre/add-pinning-attributes
branch
from
9b77639
to
ee04d8a
Compare
Doing so after discussion of changing must be recursively pinned to must be deserializable. |
|
Just for reference - curious why we're moving these here from the LMS? I'm assuming its so they can be used in D2L.Core or something? |
That is the primary driver. It could be handled in another way, but it also does enforce that the analyzers run on all the LMS libraries in a way that's not easily / accidentally undone. |
Co-authored-by: Matt Jones <[email protected]>
|
Has this PR been descoped? It looks like the analyzer is no longer included |
Yes, as per my comment on the 11th due in part to the renaming of the one attribute. |
This could be broken up into a couple PRs. Either way we'll want bring in the analyzer before switching the attributes and removing the analyzers from the LMS. The recursive pinned analyzer in the LMS isn't where we'll want it to be, due to not incorporating changes that prevent it from erroring immediately so we can remove that from the LMS before bring in the new one.
I also refactored what's in the LMS repo to clean it up a bit and start from the symbol rather than the syntax node.