How do you feel about adding Code Analysis?

[Glenn Hoeppner]

Having recently become aware of this project, I was considering committing some time to it.

I pulled in master and fired up FxCop (Static Code Analysis) with my admittedly restrictive ruleset.

As you might expect for an incomplete project, it found a lot to complain about. Much of it revolves around code that isn't implemented. Some of it are warnings that most developers probably wouldn't care about. But it did find a number of issues that look relevant. I'm not familiar at all with the code base here, so it's difficult for me to weed out false positives with ten minutes of looking, but it appears as though there are a number of problems with the code sucked in from Irony.

For example:

• The context parameter isn't used in BeginMatch() or CompleteMatch() (or in a number of other methods for that matter)
  https://github.com/Pash-Project/Pash/blob/master/Libraries/Irony/Irony/Parsing/Terminals/CommentTerminal.cs#L78-110
  
• Here's an even worse example of an unused parameter
  https://github.com/Pash-Project/Pash/blob/master/Libraries/Irony/Irony/Parsing/Terminals/TerminalFactory.cs#L160
  
• StringWriter should be disposed of
  https://github.com/Pash-Project/Pash/blob/master/Libraries/Irony/Irony/Parsing/Parser/ParseTreeExtensions.cs#L27

Anyway, you get the idea. There are a lot of warnings about methods not marked as static, not validating arguments of public methods, not specifying format providers. That sort of thing.

It probably wouldn't hurt to enable a few of the more important rules and build off that depending on how it goes.

-Glenn

[Jay Bazuzi]

Hey Glenn, thanks for looking in to this. I'm a big fan of static analysis.

One of the problems we have with applying SA tools to Pash is the fact that we are cross-platform. We have contributors working in Windows, Mac, and Linux. Our CI builds are on Windows and Linux. Some people use Xamarin Studio or Visual Studio. 

I want developers to be able to validate their changes right on their own development machines, instead of only finding out about issues when the CI build runs. I'm not sure how to make that work with FxCop. 

We hit the same problem with StyleCop, Coverity Scan, ReSharper Code Cleanup, ReSharper Inspect Code. I expect that at some point I'll have to compromise on my principle, and just let the CI do this work.

There are similar challenges around using NuGet and Chocolatey off-Windows. (Ref: http://jbazuzicode.blogspot.com/2014/05/automating-resharpers-inspect-code.html)

-J

--
You received this message because you are subscribed to the Google Groups "Pash Project" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pash-project...@googlegroups.com.
Visit this group at http://groups.google.com/group/pash-project.

[Glenn Hoeppner]

Yeah, I understand it would be a challenge. I think in the case of Pash Shell, it'd be best to just enable errors as warnings and let those of us with access to the correct tools deal with them as time allows.

-Glenn

[Jay Bazuzi]

We use two different CI systems. Travis-CI for Linux, and AppVeyor for Windows. If you can get FxCop running in AppVeyor, I think we should take it.

BTW, I am not terribly interested in static analysis for Irony, as it's a 3rd party library.

-J

[Glenn Hoeppner]

What edition of Visual Studio is installed on the Build Agent?

-Glenn

--
You received this message because you are subscribed to a topic in the Google Groups "Pash Project" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/pash-project/WaUZD1tNiX0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to pash-project...@googlegroups.com.

[Jay Bazuzi]

I'm not sure. It's probably buried somewhere in http://www.appveyor.com/docs, but I couldn't find it.

-J

[Glenn Hoeppner]

I have a branch I'd like to push up to test, but don't have access to do so.

I've never contributed to something I don't co-own, so it's possible I'm doing something wrong, but I don't think so.

-Glenn

[Jay Bazuzi]

Start by forking the Pash project in GitHub. Push your changes to that repo. 

Then make your own AppVeyor account for that repo, and experiment there.

-J

[Glenn Hoeppner]

Okay, I have it working in a forked-branch. I enabled Microsoft's basic rules on all the non-test builds. You can see that they show up in the build agent's log.

I could alter the rule set to break the build, but as mentioned earlier, this is a somewhat unique situation because the solution builds on multiple platforms. However, it wouldn't be difficult to make them break on the build agent if we wanted.

Kind of the bigger question right now is which rules to enable? As I mentioned before, because there's so much incomplete code a lot of the warnings are generated by that. I'd almost like to just turn them all on, and let contributors fix as they feel necessary.

Any thoughts?

-Glenn

[Jay Bazuzi]

I would disable all rules that are currently failing, then fail the build, then merge that to master.

Then we can incrementally enable-and-fix.

-J

[Glenn Hoeppner]

Okay, I have all that done in my local branch. But before I make a pull request, I'd like to write up some documentation around FxCop: how to enable on new projects and how to suppress false-alarms, that sort of thing.

It doesn't look like there's a web site associated with the project, but that's okay because I'm accustomed to keeping documentation as markdown in the repo.

I'd like to add a few screenshots with my documentation, and I don't want to clutter things up. How do you feel about having a /docs and /docs/images in the repo for this type of documentation?

-Glenn

[Jay Bazuzi]

There is a user-facing web site (https://pash-project.github.io/), but at the moment just about every suer is also a developer.

I think adding info to the repo is fine. Two thoughts:

- Don't use an abbr. for "docs". I may be the only person that calls my source directory "Source" and not "src". 

- Don't repeat information that is already available from official sources on the internet. We don't want an additional maintenance burden for something that isn't the core value of this project.

These aren't hard rules; use your best judgement.

-J

[Glenn Hoeppner]

Just to close the loop for anyone following this thread, I opened pull request 346 to enable/document code analysis as laid out in this thread.