-
Notifications
You must be signed in to change notification settings - Fork 260
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
Code analyzers feature #328
Comments
@dtchepak, I'm sure the xunit examples are great, but you might also enjoy looking at the FakeItEasy Analyzer source. Of course the configuration syntax is different, but we (among other things) detect attempted configuration of non-virtual members! |
@dtchepak I did a PoC for an analyzer detecting setup of non-virtual calls. You can find it here. It is able to detect non-virtual calls for following syntaxes:
code analysys for indirect substitute usage e.g
is rather difficult to process as it requires creating a data flow analysis, that is why I don't analyze those cases yet - I just make sure that I don't report false positives. If you are interesed we can move this code to NSubstitute repository - with proper PR, CodeReview etc. Of course some more manual testing is required before potential release. I have quite a few tests in place, but probably it would be good to have a tests against different versions of VS |
Hi @tpodolak, This looks awesome! If I fork this to the NSubstitute org are you happy to continue working on it there? Or we can keep it under your name if you prefer (seeing you've done all the work on it 😄). Please let me know how you'd like to run/incorporate the project in terms of releases, feature requests, collaborators/repo access etc. Let me know if there is anything I can help out with in the project (more than happy to review the code, but please keep in mind I have no Roslyn experience so I may not be very insightful 😂 ). |
Hi @dtchepak |
@tpodolak Forked to https://github.com/nsubstitute/NSubstitute.Analyzers. You've got full access -- I consider it as your repo but it's good to have it in the NSubstitute organisation so this stuff is kept together. 😄 Welcome to the team! 😄 |
Are FakeItEasy analysers shipped in the main package? There are separate packages listed? |
@dtchepak, yeah, the FakeItEasy Analyzers are shipped in separate packages, but we do include the source in the main FakeItEasy repo. |
Thanks @blairconrad! 😄 |
@dtchepak thanks for the warm welcoming. One question about building the fork on CI. Do you have some organization account on AppVeyor or TravisCI where the analyzer build should be configured or I rather should build that on my AppVeyor account? |
@tpodolak We build using https://ci.appveyor.com/project/NSubstitute/nsubstitute. I'm not sure how it all works to be honest. 😕 😊 @alexandrnikitin are you able to assist please? |
Build works as expected. I am about to push first beta package to the nuget (unfortunatelly without GH-1), should I push it as an organization or just me as owner? |
@alexandrnikitin Nuget username is the same as here - tpodolak. One more thing, I was wrong about the build, I mean the build is running but there are some missing env variables there - see this pr nsubstitute/NSubstitute.Analyzers#3. I can fix it but I cannot access the settings page. Can you grant me an access to the appveyour build? |
@tpodolak @alexandrnikitin I just created a NuGet organisation for NSubstitute and attempted to give everyone access. @tpodolak I've also tried to give you settings access to AppVeyor. Please let me know if there is any problem with accessing either NuGet or AppVeyor. |
We have the first analysers release! https://www.nuget.org/packages/NSubstitute.Analyzers.CSharp/ Will close this issue; future analyser requirements can be added to that project: (I wonder if it would be better to keep all issues here instead? 🤔) |
Just thought about your comment that NSubstitute syntax has issues with non-virtual members and realized that NSubstitute could be shipped with Roslyn analyzers. You can take a look at
xunit
as an example. They ship a set of analyzers for their assertion library, to use the assertions correctly (e.g.Assert.True(x)
instead ofAssert.Equal(true, x)
).You could also create analyzes for the most known issues:
Returns()
after a non-virtual method.Arg.Any()
.That would be awesome as it will perform analysis during the compilation and produce warnings if any issue found. On other hand, user could easily suppress warnings, if they are not relevant.
@dtchepak @alexandrnikitin Have you already thought about this idea?
The text was updated successfully, but these errors were encountered: