Initial code analyzer for Microsoft.ML, use limited StyleCop #557

TomFinley · 2018-07-19T07:11:14Z

Fixes #553.

Some WIP items:

Fix issue with new declarations on members not being properly handled by accessibility modifier check.
Enable checking in all source projects, not just Microsoft.ML.Core. (Done only there for now just to validate approach.)

Adds code analysis initially for correct usage of common Contracts.Except/Check patterns, naming conventions, variable usage and initializations, access modifiers, and other idioms used throughout the Microsoft.ML codebase. Enables analysis on Microsoft.ML projects. Uses existing StyleCop analyzer rules for these where appropriate.

TomFinley · 2018-07-19T16:26:38Z

Maybe this should be named something like internal analyzer, since there is no intent that it be used external to this project... another style of analyzer is possible for users of the API, that helps them correctly use this API, offers help and whatnot.

Edit: Of course the existence of such a thing as that is purely hypothetical, and we can easily change the name of the project if necessary. Still, maybe .NET folks have input on best practices here...

ericstj · 2018-07-19T16:45:19Z

I would indeed give this a name or put it under a folder that made it clear that it was not intended to be something that was an output of the repo. In a couple other repos we use the tools-local convention for build infra built from the same repo which consumes it.

ericstj · 2018-07-20T15:46:40Z

tools-local/Microsoft.ML.CodeAnalyzer/Microsoft.ML.CodeAnalyzer.csproj

@@ -1,12 +1,11 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFramework>netstandard1.3</TargetFramework>


An other option (if you need dependencies) would be to target the concrete TFM (either net4x for desktop msbuild, or netcoreapp2.x for CLI-based msbuild, or cross-target to support both) and direct the SDK to copy those dependencies to the output folder. I believe the default is to not copy dependencies for netcoreapp and netstandard.

PS: To be clear, I am not suggesting you copy dependencies for netstandard, that's an anti-pattern as netstandard isn't a runnable framework and you should never be producing a runnable layout for a netstandard thing unless you have complete knowledge of all dependencies and know that they don't require more TFM-specific runtime libraries on the framework you'll run on.

Hi @ericstj , I don't think I need any dependencies, at least, aside from those directly needed by any C# Roslyn analyzer. The original analyzer I was porting was actually .NET Standard 1.3 anyway, so this is, I guess, fine?

(TBH I don't actually have any idea why this error was occurring. This problem of IDE1003 was only reproing on my home computer, not my work computer. Which makes no sense to me, since AFAIK my Visual Studio installations on the two are more or less the same.)

I see, then yeah this is fine for now. netstandard1.3 had inbox support without any out-of-band facades on net46. That's why it works well here.

netstandard1.5 had some out-of-band facades required for support on [net461-net47] which is likely why your home computer was failing. It probably didn't have net471 or later, but your work machine does.

ericstj · 2018-07-20T15:48:23Z

tools-local/Microsoft.ML.CodeAnalyzer/ModifierFixProvider.cs

+                case SyntaxKind.EventDeclaration:
+                    var evDec = (EventDeclarationSyntax)parent;
+                    RegisterFix(context, diagnostic, evDec.Modifiers, evDec.WithModifiers, privateAc, evDec);
+                        break;


nit indentation.

ericstj · 2018-07-20T16:18:01Z

src/Directory.Build.props

@@ -14,4 +14,12 @@

  </PropertyGroup>

+  <ItemGroup>


You may want add a condition like Condition="'$(UseMLCodeAnalyzer)' != 'false'" so that projects could opt out. Supporting such a condition requires this to be in Directory.Build.targets so that it is imported after the project body where a project might set this property.

Supporting such a condition requires this to be in Directory.Build.targets so that it is imported after the project body where a project might set this property.

All MSBuild properties are evaluated first, then Items are evaluated in a second pass. So it doesn't necessarily need to be in a .targets file.

Actually this was something I was curious about with how MSBuild works, and I couldn't find the documentation on this... so let me see if I have this right @eerhardt and @ericstj ... so imported projects (including implicitly imported projs like Directory.Build.props, their properties (that is, I suppose, things in PropertyGroup tags) are evaluated before the local importing project file, but their items (things in ItemGroup tags) are evaluated after the local importing project file? Is that so?

all MSBuild properties are evaluated first, then Items are evaluated in a second pass.

Right you are. Still for sanity's sake its usually better to follow logical order.

@TomFinley here are the docs: https://msdn.microsoft.com/en-us/library/dd997067.aspx#Anchor_2. Like @eerhardt said you don't necessarily need to move it when adding the condition, but I like to do that by convention for sanity's sake. In general I try to use props files only when I need to define something that a project might need to consume itself or I need it defined before some other targets are imported. Also if you want a visual representation of the import resolution you can run /pp on the project file (msbuild myproject.csproj /pp:project.pp).

Ah OK... so all properties are evaluated in the order in which they occur no matter where they may be, then all items, I guess? And the Directory.Build.props (if found) is always implicitly imported first? That begins to make some sense, I think.

Directory.Build.props (and targets) are just imported by convention in the SDK props/targets, which get imported before/after the project body respectively. /pp output shows you precisely where (which is sometimes relevant when trying to override).

eerhardt · 2018-07-20T16:34:35Z

How many of these things can be handled by an .editorconfig file?

https://docs.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options

For example, in https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference, it has:

.NET code style settings
* "This." and "Me." qualifiers
  * dotnet_style_qualification_for_field
  * dotnet_style_qualification_for_property
  * dotnet_style_qualification_for_method
  * dotnet_style_qualification_for_event

TomFinley · 2018-07-20T17:16:19Z

Hi @eerhardt , .editorconfig would be quite positive, and I'll add it, but is there a way to make the things in editorconfig actually show up as errors? From what I see it's mostly for autoformatting, and considering how often in PRs I have to mention "please, please, please don't forget to hit ctrl-k-d (indeed @ericstj had to do that to me this morning since I had screwed up indenting 😄) I wonder if that will be, by itself, sufficient. I suspect not.

Actually what I was more surprised about is that some of these things, it seems like should be findable through existing analyzers (e.g., on member modifiers, surely there must be a rule somewhere for putting new first, then having explicit access modifiers), but I couldn't quite find them.

Edit: Still figuring this thing out. Nice little enhancement.

Edit: Seems positive, I might prefer we discuss what to put in this file in a separate PR since that seems like something that merits an independent discussion.

sharwell · 2018-07-20T17:39:56Z

src/Microsoft.ML.Api/SchemaDefinition.cs

@@ -287,9 +287,9 @@ public void RemoveMetadata(string kind)
        /// </summary>
        public Column this[string columnName]
        {
-#pragma warning disable TLC_NoThis // Do not use 'this' keyword for member access
+#pragma warning disable MSML_NoThis // Do not use 'this' keyword for member access


📝 This analysis is available through a couple of alternatives, which may eliminate sources of bugs.

Replaced with SX1101.

sharwell · 2018-07-20T17:40:32Z

src/Microsoft.ML.Core/Data/ColumnType.cs

@@ -496,7 +496,7 @@ public static NumberType Float
            get { return R4; }
        }

-        public new static NumberType FromKind(DataKind kind)
+        new public static NumberType FromKind(DataKind kind)


❕ The previous ordering is definitely preferred by a majority of projects. Would encourage correcting this analysis.

📝 Accessibility ordering analysis is available through a couple of maintained alternatives, which should help avoid analysis bugs long-term.

Are you sure? All samples I see with this tend to prefer new first...

The IDE aligns features on the following total order by default:
https://github.com/dotnet/roslyn/blob/a1b1ae53dfa5763cf2bc557524ac3ec78b05e95d/src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs#L145-L156

Alternative approaches such as StyleCop defined a partial order which is consistent with the IDE defaults.

I've seen some requests over to deviate in different ways, but not with respect to the new modifier.

All right sounds fine. Replaced with SC rules SA1205, SA1206, SA1207, SA1212, SA1400.

sharwell · 2018-07-20T17:43:23Z

src/Microsoft.ML.Data/EntryPoints/InputBuilder.cs

@@ -791,7 +791,7 @@ public static class Range
        /// </summary>
        public static class Deprecated
        {
-            public new static string ToString() => "Deprecated";
+            new public static string ToString() => "Deprecated";


😕 (unrelated to the change, and looking specifically at the member Deprecated.ToString()) This is super weird. How is it used?

It is very strange. So, there is some scheme for defining an "entry-point graph" which is a structure defined as a JSON graph which can be serialized and sent across the wire. So there is this utility class here for holding the standard JSON property names... that is fine, so far as it goes, assuming we accept the idea of entry-points being structured in this incredibly weakly typed fashion. But let's set that aside. Now then we could have had a property named JsonPropertyName or something on this, why the devil would we among all possible names choose ToString specifically, which requires this odd hiding? That seems like at best a poor choice.

I'd prefer though not to deal with these odd things in this PR.

sharwell · 2018-07-20T17:44:57Z

src/Microsoft.ML.FastTree/Training/DcgPermutationComparer.cs

@@ -44,10 +44,10 @@ public static DcgPermutationComparer GetDcgPermutationFactory(string name)
    /// </summary>
    public class DescendingStablePessimisticPermutationComparer : DescendingStablePermutationComparer
    {
-#pragma warning disable TLC_GeneralName // The naming is the least of this class's problems. A setter with no getter??
+#pragma warning disable MSML_GeneralName // The naming is the least of this class's problems. A setter with no getter??


The naming is the least of this class's problems. A setter with no getter??

💡 Comments like this should be in issues, not comments in source code.

TBH when it comes to the FastTree project's structuring, I don't even know where to start with its problems.

sharwell · 2018-07-20T17:47:57Z

tools-local/Microsoft.ML.CodeAnalyzer/WhitespaceAnalyzer.cs

+namespace Microsoft.ML.CodeAnalyzer
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class WhitespaceAnalyzer : DiagnosticAnalyzer


💡 Consider using one of the maintained alternatives instead of roll-your-own analyzer for this.

Great. Point me at one?

StyleCop.Analyzer I see. Let's see if I can get that to work...

Replaced with SC rules SA1028 and SA1507. Incidentally this change is accompanied by many changes to files because this also covers XML doc comments.

sharwell · 2018-07-20T17:50:06Z

tools-local/Microsoft.ML.CodeAnalyzer/NoThisAnalyzer.cs

+namespace Microsoft.ML.CodeAnalyzer
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class NoThisAnalyzer : DiagnosticAnalyzer


💡 Consider using one of the maintained alternatives instead of roll-your-own analyzer for this.

Point me at one please. Same for all your identical comments.

Replaced with SX1101.

sharwell · 2018-07-20T17:50:39Z

tools-local/Microsoft.ML.CodeAnalyzer/ModifierFixProvider.cs

+namespace Microsoft.ML.CodeAnalyzer
+{
+    [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(ModifierFixProvider)), Shared]
+    public sealed class ModifierFixProvider : CodeFixProvider


💡 Consider using one of the maintained alternatives instead of roll-your-own analyzer for this.

Replaced with SC rules SA1205, SA1206, SA1207, SA1212, SA1400.

sharwell · 2018-07-20T17:50:53Z

tools-local/Microsoft.ML.CodeAnalyzer/ModifierAnalyzer.cs

+namespace Microsoft.ML.CodeAnalyzer
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class ModifierAnalyzer : DiagnosticAnalyzer


💡 Consider using one of the maintained alternatives instead of roll-your-own analyzer for this.

Replaced with SC rules SA1205, SA1206, SA1207, SA1212, SA1400.

sharwell · 2018-07-20T17:51:03Z

tools-local/Microsoft.ML.CodeAnalyzer/InterfaceNameAnalyzer.cs

+namespace Microsoft.ML.CodeAnalyzer
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class InterfaceNameAnalyzer : DiagnosticAnalyzer


💡 Consider using one of the maintained alternatives instead of roll-your-own analyzer for this.

Replaced with SC rule SA1302.

sharwell · 2018-07-20T17:51:50Z

src/Directory.Build.props

+      Condition="'$(UseMLCodeAnalyzer)' != 'false'"
+      Include="$(MSBuildThisFileDirectory)\..\tools-local\Microsoft.ML.CodeAnalyzer\Microsoft.ML.CodeAnalyzer.csproj">
+      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
+      <OutputItemType>Analyzer</OutputItemType>


😎 First time I've seen this used in practice. Very cool if it works.

sharwell · 2018-07-20T17:54:14Z

@eerhardt @TomFinley @ericstj Feel free to set up a meeting to talk about any issues you hit with this. I'm on the team that builds this experience in the IDE. 😄

sharwell · 2018-07-20T19:00:31Z

but is there a way to make the things in editorconfig actually show up as errors?

The following table shows the support for this in the 15.8 release:

	Formatting (whitespace issues)	Other code style
IDE behavior	dotnet/roslyn#7067	✔️
Compile-time behavior	https://github.com/dotnet/roslyn/projects/18	(same ⬅️)

We are working to implement the full matrix, offering users a great overall experience for defining and enforcing code style policies in their solutions. The links currently in the table point to tracking issues related to the feature.

TomFinley · 2018-07-20T22:25:21Z

Hi @sharwell, so just to check, if I have a config like this:

trim_trailing_whitespace = true : error

It will provide an error in 15.8, with this .editorconfig validation during compilation, even in msbuild (that is, not just in Visual Studio itself, I hope)? If so that's pretty exciting.

Adds code analysis initially for correct usage of common Contracts.Except/Check patterns, naming conventions, variable usage and initializations, access modifiers, and other idioms used throughout the Microsoft.ML codebase. Enables analysis on Microsoft.ML projects.

…easier editing

* Disable analysis for generated code * Fix found issues

…DE1003. While builds work fine both inside and outside of Visual Studio, and VS does not complain on my work computer, VS on my home computer complains with IDE1003. Following the advice here, the analyzer will use .NET Standard 1.3, which requires only minor adjustments. dotnet/roslyn#22368 My understanding was that VS should support 2.0 referencing analyzers, but seemingly that is incorrect.

…types.

… project property to false.

sharwell · 2018-07-24T12:32:48Z

trim_trailing_whitespace

I would encourage not using this setting. It is not aware of things like code generation tools that include trailing whitespace in the generated code (unnecessary repository churn) or language features like verbatim string literals (can break code). Currently .editorconfig does not have a setting for checking trailing whitespace.

It will provide an error in 15.8, with this .editorconfig validation during compilation, even in msbuild (that is, not just in Visual Studio itself, I hope)?

No, this is part of the "compile-time behavior" row which is not yet supported. The links point to issues tracking our ongoing work on the subject.

TomFinley · 2018-07-24T18:21:28Z

K. While it seems positive, I'll leave the definition for .editorconfig for a separate issue and PR.

TomFinley · 2018-07-24T18:24:25Z

The latest round up updates involve separate commits for every rule I was able to replace with a SC replacement. Some of the SC rules are close-but-not-quite-what-we-already-do. (E.g., there are naming rules, but they don't quite exactly line up.) While I am open to changing our existing styles, I'd rather not make that discussion part of this specific PR.

For the sake of the SC introduction, it may be helpful to view the diffs from individual commits, since there are lots of files that are incidentally modified as we change them to conform to the style. E.g.: Introduce SC, rely on it for "this" check might be the first commit to look at where we start using commits.

eerhardt · 2018-07-24T23:07:20Z

src/Microsoft.ML/LearningPipelineDebugProxy.cs

@@ -225,7 +225,12 @@ internal class PipelineItemDebugColumn
    {
        public string Name { get; set; }
        public string Type { get; set; }
-        public string SlotNames { get; set; } = string.Empty;
+        public string SlotNames { get; set; }


I'm not sure I fully understand the reasoning behind this rule. Can we add the reasoning to the rule description? Right now it just says

All instance fields or properties should be initialized in a constructor, not in the field

Sure, added this

eerhardt · 2018-07-24T23:08:15Z

src/Microsoft.ML.Api/SchemaDefinition.cs

@@ -287,9 +287,9 @@ public void RemoveMetadata(string kind)
        /// </summary>
        public Column this[string columnName]
        {
-#pragma warning disable TLC_NoThis // Do not use 'this' keyword for member access
+#pragma warning disable MSML_NoThis // Do not use 'this' keyword for member access


Is this still valid? We removed our own rule here, right?

Whoops, missed one...

eerhardt · 2018-07-24T23:11:47Z

src/Microsoft.ML.Data/DataView/Transposer.cs

@@ -1063,7 +1063,7 @@ public override string GetColumnName(int col)
                    return _view.Schema.GetColumnName(SrcCol);
                }

-                public override abstract ColumnType GetColumnType(int col);
+                public abstract override ColumnType GetColumnType(int col);


Should this just be removed? It is already abstract in the base class.

eerhardt · 2018-07-24T23:15:08Z

src/Microsoft.ML.FastTree/TreeEnsemble/RegressionTree.cs

@@ -26,7 +26,7 @@ public class RegressionTree
        // Weight of this tree in the ensemble

        // for each non-leaf, we keep the following data
-        public Float[] _defaultValueForMissing;
+        public Float[] DefaultValueForMissing;


Should this be a property instead? Potentially a readonly property?

eerhardt · 2018-07-24T23:17:14Z

src/Microsoft.ML.PipelineInference/AutoInference.cs

-                ectx.CheckValue(nodes[0], nameof(nodes), "Empty Subgraph");
-                ectx.CheckValue(nodes[0][FieldNames.Inputs], "Inputs", "Empty subgraph node inputs.");
+                ectx.Check(nodes != null, "Empty Subgraph");
+                ectx.Check(nodes[0] != null, "Empty Subgraph");


Having the same message as the above line makes it hard to diagnose which one of these checks actually failed. Was nodes null? or Was nodes[0] null?

eerhardt · 2018-07-24T23:19:40Z

tools-local/Microsoft.ML.CodeAnalyzer/ContractsCheckAnalyzer.cs

+
+        private const string Category = "Contracts";
+
+        public static class NameofDiagnostic


Do these nested classes need to be public?

I wasn't quite sure about this one. I guess there's no real point in making the outside names accessible in this way, since you could just call get supported diagnostics anyway.

eerhardt · 2018-07-24T23:22:02Z

tools-local/Microsoft.ML.CodeAnalyzer/Microsoft.ML.CodeAnalyzer.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="2.8.2" />


Since all our projects (including our .nupkgproj projects) have a reference to this .csproj, we should ensure we are not getting dependencies on these packages in our nuget packages, and that our built .dlls don't have references to these assemblies (and that they don't show up in Intellisense at all, so no one makes the mistake of referencing these assemblies).

I had the same thought. I trust MSBuild and VisualStudio will do the right thing with this, but I don't trust NuGet.

Hi @eerhardt that's a good point. As near as I can tell things are working as we like. If I do build.cmd -release -buildPackages, and I unzip the main Microsoft.ML nuget, and do something like [Reflection.Assembly]::LoadFrom("...").GetReferencedAssemblies() on the usual suspects (Microsoft.ML.Core.dll and Microsoft.ML.Data.dll and the like) I do not see either the project for the analyzer or StyleCop. So, I think everything is working as desired, and these are analyzer-only references as intended?

Perfect. Also, check the .nuspec file in the Microsoft.ML nuget package doesn't have these dependencies either. If not, then I think we are good to go.

eerhardt

Just some minor nits, questions and comments.

ericstj

Looks ok to me. I've only skimmed the diffs but have reviewed the infra. We can tune it over time to have the correct rules.

ericstj · 2018-07-25T18:36:42Z

src/Directory.Build.props

  </PropertyGroup>

+  <ItemGroup>
+    <ProjectReference
+      Condition="'$(UseMLCodeAnalyzer)' != 'false'"


You might also want a project type condition '$(MSBuildProjectExtension)' == '.csproj'.

That's a good idea

OK I have added this as a condition to both the stylecop and project import @ericstj

) * Initial code analyzer for Microsoft.ML Adds code analysis initially for correct usage of common Contracts.Except/Check patterns, naming conventions, variable usage and initializations, and other idioms used throughout the Microsoft.ML codebase. Enables analysis on Microsoft.ML projects. Also added StyleCop, with most rules currently disabled, but rules on whitespace, declaration modifier ordering, explicit access modifiers, and inteface naming check. * Analyzer is .NET Standard 1.3 to avoid problems with IDE1003. dotnet/roslyn#22368 * Projects in `src` can not use analyzer by setting `UseMLCodeAnalyzer` project property to false.

TomFinley requested review from Ivanidzo4ka, eerhardt, ericstj and Zruty0 July 19, 2018 07:11

TomFinley mentioned this pull request Jul 19, 2018

Introduce code analyzer #553

Closed

TomFinley force-pushed the tfinley/RoboTom branch from fc9cc3e to 93f8556 Compare July 19, 2018 23:31

TomFinley changed the title ~~WIP Initial code analyzer for Microsoft.ML~~ Initial code analyzer for Microsoft.ML Jul 19, 2018

TomFinley force-pushed the tfinley/RoboTom branch from 93f8556 to c147d77 Compare July 19, 2018 23:41

ericstj reviewed Jul 20, 2018

View reviewed changes

sharwell reviewed Jul 20, 2018

View reviewed changes

TomFinley added 11 commits July 23, 2018 18:45

Movement of the larger test sources into separate resource files for …

bc3edf2

…easier editing

Expand modifier analyzer to handle "new" modifier correctly

d19adb1

Move "analyzers" folder to "tools-local"

d98092f

* Enable analysis on all files

1faa44c

* Disable analysis for generated code * Fix found issues

Fix sln

19a4993

Modifier

89362cd

Fix problems with modifier analyzer on event and indexer syntax node …

dde0ae7

…types.

Indentation on break in mod fix provider

ba91e6b

Projects in src can not use analyzer by setting UseMLCodeAnalyzer…

bc12f31

… project property to false.

TomFinley force-pushed the tfinley/RoboTom branch from a2ee5dd to bc12f31 Compare July 24, 2018 13:54

TomFinley changed the title ~~Initial code analyzer for Microsoft.ML~~ Initial code analyzer for Microsoft.ML, use limited StyleCop Jul 24, 2018

TomFinley added 4 commits July 24, 2018 11:26

Introduce SC, rely on it for "this" check

a3d703a

Whitespace analyzer change

f27fbda

Declaration mods ordering and explicit access modifiers

ad638eb

Interface name check

0b1e6b7

TomFinley force-pushed the tfinley/RoboTom branch from 681bd56 to 0b1e6b7 Compare July 24, 2018 18:26

eerhardt reviewed Jul 24, 2018

View reviewed changes

eerhardt approved these changes Jul 24, 2018

View reviewed changes

ericstj approved these changes Jul 25, 2018

View reviewed changes

Review comments

d3d6fbd

TomFinley merged commit 5e08fa1 into dotnet:master Jul 25, 2018

TomFinley deleted the tfinley/RoboTom branch July 25, 2018 23:19

eerhardt mentioned this pull request Jul 31, 2018

Expose the x86 HWIntrinsics via a set of class hierarchies matching the underlying ISA hierarchies dotnet/coreclr#19186

Merged

ghost locked as resolved and limited conversation to collaborators Mar 29, 2022


		private const string Category = "Contracts";

		public static class NameofDiagnostic

Initial code analyzer for Microsoft.ML, use limited StyleCop #557

Initial code analyzer for Microsoft.ML, use limited StyleCop #557

Uh oh!

Conversation

TomFinley commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Jul 19, 2018

Uh oh!

ericstj Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jul 20, 2018

Uh oh!

TomFinley commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TomFinley commented Jul 19, 2018 •

edited

Loading

TomFinley commented Jul 19, 2018 •

edited

Loading

ericstj Jul 20, 2018 •

edited

Loading

ericstj Jul 20, 2018 •

edited

Loading

TomFinley commented Jul 20, 2018 •

edited

Loading

sharwell Jul 20, 2018 •

edited

Loading

sharwell Jul 20, 2018 •

edited

Loading

sharwell commented Jul 20, 2018 •

edited

Loading

sharwell commented Jul 24, 2018 •

edited

Loading

TomFinley commented Jul 24, 2018 •

edited

Loading