Fix attribute comparison for ProxyGenerator cache #219

thomaslevesque · 2016-10-13T00:39:22Z

Fixes #77

At this point, this PR is mostly a proof of concept; I didn't try to preserve the existing API for backwards compatibility, I just changed the type of AdditionalAttributes from CustomAttributeBuilder to a new CustomAttributeInfo class. This class has the same constructors as CustomAttributeBuilder, and overrides Equals and GetHashCode. I also introduce a CustomAttributeInfo.FromExpression method to make it easier to define attributes:

var info = CustomAttributeInfo.FromExpression(
    () => new MyAttribute("foo") { Value = 42, Type = typeof(int) });
options.AdditionalAttributes.Add(info);

jonorossi · 2016-10-20T16:01:38Z

@thomaslevesque thanks, it looks good, sorry for the slow response to review this. I do like FromExpression, much better than the now obsolete code that pulls apart an actual Attribute instance to try rebuilding it.

Now to discuss whether this should be a breaking change. Looking at the 3 main mocking libraries only FakeItEasy uses AdditionalAttributes. You guys expose CustomAttributeBuilder via
IFakeOptions.WithAdditionalAttributes, are you happy for that to be a breaking change to FakeItEasy and are you comfortable exposing a DynamicProxy type on your IFakeOptions interface? If you are ILMerging you'll also have to expose that type.

CustomAttributeInfo works, however this all makes me wonder whether we should just side step this and inspect CustomAttributeBuilder.m_con (ctor on mono) and CustomAttributeBuilder.m_blob (data on mono) using reflection. It'll be a faster comparison, however the IL blob will preserve property order from the code which is probably want we want here anyway. We already use reflection to access many public members of the BCL types. Both options aren't ideal, would love to hear your (and anyone else's) thoughts.

thomaslevesque · 2016-10-20T21:01:08Z

are you happy for that to be a breaking change to FakeItEasy

I don't mind the breaking change. It's not a very widely used feature, so it shouldn't affect many users, and the fix will be pretty easy anyway. @adamralph, @blairconrad, what do you think?

and are you comfortable exposing a DynamicProxy type on your IFakeOptions interface? If you are ILMerging you'll also have to expose that type.

That's a good question, I hadn't thought about that. We already expose part of the DynamicProxy API as a result of using ILMerge, but not directly as part of the FakeItEasy API. I'd rather avoid that if possible, but I'm not sure how. I guess we could create yet another CustomAttributeBuilder-like type that would be specific to FakeItEasy, but it's not a very appealing option. Or we could expose only a method that accepts an Expression<Func<Attribute>>.

CustomAttributeInfo works, however this all makes me wonder whether we should just side step this and inspect CustomAttributeBuilder.m_con (ctor on mono) and CustomAttributeBuilder.m_blob (data on mono) using reflection. It'll be a faster comparison, however the IL blob will preserve property order from the code which is probably want we want here anyway. We already use reflection to access many public members of the BCL types. Both options aren't ideal, would love to hear your (and anyone else's) thoughts.

I guess that would work too, and it would have the benefit of avoiding the breaking change entirely. Of course, as always when using reflection on private members, there's the risk of the internals being changed in a BCL update, but I don't think it's very likely.

thomaslevesque · 2016-10-20T21:16:55Z

Slightly off-topic, but related: while working on this I noticed the obsolete AttributesToAddToGeneratedTypes property. Do you intend to remove it in 4.0?

blairconrad · 2016-10-21T02:13:00Z

I haven't looked at the changes in depth, but in general it looks good to me.
I love FromExpression (surprised no test coverage for it, though).

I don't mind the breaking change. It's not a very widely used feature, so it shouldn't affect many users, and the fix will be pretty easy anyway. @adamralph, @blairconrad, what do you think?

Well worth it, in my opinion. I'd go for the breaking change.

Or we could expose only a method that accepts an Expression<Func<Attribute>>

Yes! This. 😻

blairconrad · 2016-10-21T02:15:06Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+					return false;
+			}
+			return true;
+		}


returns false if x has members not in y, but not the other way 'round, no?

I think most of the comparisons can be short-circuited by checking the length of all the collections before even checking any contents, that would fix this problem.

jonorossi · 2016-10-21T02:51:24Z

Slightly off-topic, but related: while working on this I noticed the obsolete AttributesToAddToGeneratedTypes property. Do you intend to remove it in 4.0?

Yep, it was marked obsolete in 2009 so it can go, I actually wrote about it in my last comment then removed the mention to avoid making things more complicated for now. If it gets in your way then remove it, otherwise lets do it separate and update our changelog.

I guess that would work too, and it would have the benefit of avoiding the breaking change entirely. Of course, as always when using reflection on private members, there's the risk of the internals being changed in a BCL update, but I don't think it's very likely.

We can add a FromExpression method either way, the remaining question is do we add CustomAttributeInfo or inspect the privates of CustomAttributeBuilder. I agree that the privates are unlikely to change (however different between the CLR and Mono) which is why I would accept the risk, however after another think/look at DP there is no precedent for accessing privates that are not DP's own or generated by DP (so under its control). I'm heavily leaning towards your CustomAttributeInfo to avoid adding tech debt that someone else has to sort out in the future, and we are about to release a major version anyway so it's a perfect time.

I think we should turn your PoC into a PR ready to merge. i.e. some unit tests, update the changelog and docs.

jonorossi · 2016-10-21T02:52:26Z

If it gets in your way then remove it, otherwise lets do it separate and update our changelog.

Actually, let's remove AttributesToAddToGeneratedTypes in this PR too, two birds one stone thingy.

thomaslevesque · 2016-10-21T07:49:55Z

Thanks for the feedback guys! I'll make the requested changes.

I love FromExpression (surprised no test coverage for it, though).

I'll add some tests, but I didn't want to waste time doing it until the idea was approved 😉

blairconrad · 2016-10-21T10:08:28Z

We can add a FromExpression method either way,

I was also thinking that we could do the same for FakeItEasy even before this PR makes it into a prime-time Castle.Core. It would mean duplicate work that would later be ripped out, but could provide a gentler introduction to the new way to specify attributes. Our clients could have a chance to convert before we make a breaking change.

jonorossi · 2016-10-21T10:53:39Z

I was also thinking that we could do the same for FakeItEasy even before this PR makes it into a prime-time Castle.Core.

Up to you, however I plan to release 4.0.0-beta2 after we get this merged. Need to sort out #200, then plan to ship a final release very soon.

thomaslevesque · 2016-10-21T21:57:25Z

I realized my implementation was incomplete, as it didn't properly support arrays as attribute arguments (attributes support primitive types, Object, Type, enums and single dimensional arrays of those types, see §17.1.3 in the C# specs). I updated the code to support this.

I also removed AttributesToAddToGeneratedTypes and related code (e.g. AttributeDisassembler seemed to exist only for this scenario, so I removed it as well).

I still need to add unit tests, but that's all for today.

BTW I'm having a hard time working on the Castle Core code base. The non-.NET Core solution doesn't build in Visual Studio 2015 because of the project.json files. The .NET Core solution builds, but running the tests in the IDE doesn't work (tried with the VS runner and the R# runner). Does it work for you @jonorossi ?

jonorossi · 2016-10-22T09:14:05Z

BTW I'm having a hard time working on the Castle Core code base. The non-.NET Core solution doesn't build in Visual Studio 2015 because of the project.json files. The .NET Core solution builds, but running the tests in the IDE doesn't work (tried with the VS runner and the R# runner).

I can't find the .NET CLI issue right now but the updated VS2015 MSBuild targets files break the csproj projects when there is a project.json file in the same directory, so I've been using VS2013, I was actually using VS2013 until someone else reported the problem a while back. My suggestion either use VS2013 or temporarily delete project.json, it is all short lived so I'm not interested in hacking our csproj files to workaround the defect. I use ReSharper's test runner in VS2013 for the .NET Framework projects, and the VS test runner in VS2015 for the .NET Core project.

adamralph · 2016-10-22T09:20:27Z

Did we make a decision about the FIE API? I don't think we should expose a Castle.Core type in our public API. We don't currently, and effectively Castle.Core is a hidden implementation detail. We exclude the three types from internalisation when IL merging, but that's only due to the mechanics of Castle.Core. We don't expose those types in our API.

jonorossi · 2016-10-22T09:35:41Z

Did we make a decision about the FIE API?

Obviously that is for you guys to ultimately decide, but I liked @thomaslevesque suggestion and that was enough of a solution for me to proceed knowing you guys can continue to keep DynamicProxy an implementation detail:

Or we could expose only a method that accepts an Expression<Func<Attribute>>.

blairconrad · 2016-10-22T11:11:49Z

@adamralph, I would also like to

expose only a method that accepts an Expression<Func<Attribute>>.

thomaslevesque · 2016-10-22T11:18:33Z

My suggestion either use VS2013 or temporarily delete project.json

Thanks for the suggestion. I no longer have VS2013, so I've been deleting the project.json and project.lock.json.

Did we make a decision about the FIE API?

Not a formal decision, but I think we all agree about not exposing a Castle Core type in our API. My preference would be to just expose the expression-based API, i.e.:

A.Fake<IFoo>(o => o.WithAdditionalAttribute(() => new MyAttribute(42, "blah")));

blairconrad · 2016-10-22T11:18:24Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+		private static readonly IEqualityComparer<object> ValueComparer = new AttributeArgumentValueEqualityComparer();
+
+		private readonly CustomAttributeBuilder builder;
+		private readonly ConstructorInfo con;


A minor thing, but I don't love "con" as a name. Perhaps its well known, it did slow me down a little. Consider "constructor"?

I don't love it either, but I chose it because it's the name used in the CustomAttributeBuilder constructor, so it seemed the natural choice since CustomAttributeInfo is meant to be a "clone" of that class. But I can change it to something else of course.

As I say, a minor thing. I'm content to go along.

blairconrad · 2016-10-22T11:20:24Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+					var assignment = binding as MemberAssignment;
+					if (assignment == null)
+					{
+						throw new ArgumentException("Only assignments bindings are supported");


"assignment bindings"

oops. I'll fix it, thanks

blairconrad · 2016-10-22T11:28:14Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+						return array;
+					}
+				}
+				throw new ArgumentException("Only constant and single-dimensional array expressions are supported");


We may reach this line in a case where a single-dimensional array expression is not supported. (When allowArray is false.)
I'd consider reworking the flow to avoid a potentially misleading exception message.
While I'm commenting on it, if such a rework avoided a boolean parameter, I'd consider it an improvement. I never know what these mean. Way back on line 113, I was all '"true"? What's "true" mean?".
But it's not such a huge deal, given that it's a private method and we can find the answer pretty quickly.

(I thought I had sent this comment, but apparently Github thought I had just started a review... weird)

We may reach this line in a case where a single-dimensional array expression is not supported. (When allowArray is false.)
I'd consider reworking the flow to avoid a potentially misleading exception message.

I thought about it, but in fact, it can only happen if we find an array inside an array. Attribute arguments support single-dimension arrays, but not nested arrays. So in the end the exception message would still be correct.

While I'm commenting on it, if such a rework avoided a boolean parameter, I'd consider it an improvement. I never know what these mean. Way back on line 113, I was all '"true"? What's "true" mean?".

In this case true means that the value is allowed to be an array. The only case where it's false is when called recursively examining array elements (to avoid nested arrays). I can rename it and/or use a named argument on the call site to make things clearer if necessary. I don't see an elegant way to avoid the bool parameter and avoid awkward duplication...

it can only happen if we find an array inside an array

Ah! Yes. Thanks. I see it now after rereading. And having you tell me. 😊

I can rename it and/or use a named argument on the call site to make things clearer if necessary.

The meaning of the parameter is quite clear on the inside of the method. And I like the name.
For me, a named argument would improve the call sites. I know they're not everyone's cup of tea, though.

An alternative would be a microtype (probably an enum) so we could have explicitly named values at the call sites, but again, maybe that's too much.

In the end, it's a private method, and not hard for anyone who's confused by the calls to look up the meaning of the bool. Thanks for listening.

blairconrad · 2016-10-22T11:35:49Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+		{
+		}
+
+		public static CustomAttributeInfo FromExpression(Expression<Func<Attribute>> expression)


I'm sure this is perfectly correct, but it would be nice to see it exercised at least once in the tests.

It will. I'm not done yet ;)

I'm sorry. I must've missed that note. I thought you'd finished it up. What a nag you must think me. Of course now I see the [WIP], which has never left.

thomaslevesque · 2016-10-22T18:22:00Z

OK, I think I'm done now. Added some tests fixed a few issues, and rebased onto master.

blairconrad · 2016-10-23T02:14:04Z

FWIW, it looks very good to me. Thanks, @thomaslevesque!

jonorossi · 2016-10-23T09:09:36Z

src/Castle.Core/DynamicProxy/ProxyGenerationOptions.cs

-		[Obsolete(
-			"This property is obsolete and will be removed in future versions. Use AdditionalAttributes property instead. " +
-			"You can use AttributeUtil class to simplify creating CustomAttributeBuilder instances for common cases.")]
-		public IList<Attribute> AttributesToAddToGeneratedTypes


Can you please update the changelog with a breaking change for two of these properties and removing IAttributeDisassembler.

jonorossi · 2016-10-23T09:11:04Z

src/Castle.Core.Tests/Castle.Core.Tests.csproj

@@ -167,6 +167,7 @@
    <Compile Include="Components.DictionaryAdapter.Tests\IPhoneWithFetch.cs" />
    <Compile Include="Components.DictionaryAdapter.Tests\NameValueCollectionAdapterTests.cs" />
    <Compile Include="DynamicProxy.Tests\AttributesToAvoidReplicatingTestCase.cs" />
+    <Compile Include="CustomAttributeInfoTestCase.cs" />


Could you move these unit tests into the DynamicProxy.Tests directory.

jonorossi · 2016-10-23T09:24:39Z

src/Castle.Core.Tests/CustomAttributeInfoTestCase.cs

+		}
+	}
+}
+	;


There is a stray semicolon at the end here.

Oops, don't know how it ended up here...

jonorossi · 2016-10-23T09:25:52Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+			this.constructorArgs = constructorArgs;
+
+			this.properties = namedProperties
+				.Zip(propertyValues, (p, v) => new { p.Name, Value = v })


Enumerable.Zip isn't available on .NET 3.5, it was added in .NET 4.0.

Argh, forgot about that... I guess I'll just use a for loop. I could also add a compatibility shim, but that would probably be overkill for that use case.

jonorossi · 2016-10-23T09:38:02Z

src/Castle.Core/DynamicProxy/CustomAttributeInfo.cs

+			this.builder = new CustomAttributeBuilder(constructor, constructorArgs, namedProperties, propertyValues, namedFields, fieldValues);
+
+			this.constructor = constructor;
+			this.constructorArgs = constructorArgs;


Should we copy the constructorArgs array to make sure this type stays immutable?

Good point. Should probably do it for the other parameters too.

Should probably do it for the other parameters too.

Scratch that. The other parameters are used to build dictionaries, not kept as is.

That was my thought at first as well. Then I had a second thought. It'll probably just reveal my inadequate grasp of what's going on here, but may not individual elements of constructorArgs or the other *Values parameters be single-dimensional arrays, which could be mutated after the fact?

thomaslevesque · 2016-10-23T19:04:36Z

@jonorossi I made the requested changes

thomaslevesque · 2016-10-23T19:06:06Z

btw @jonorossi let me know if you want me to squash some or all the commits before the merge. I don't know how you usually handle this in Castle.Core

thomaslevesque · 2016-10-23T19:44:35Z

may not the other parameters (or even individual args of constructorArgs) be single-dimensional arrays, which could be mutated after the fact?

It doesn't matter for namedProperties, propertyValues, namedFields, and fieldValues, because we don't store them; we just create dictionaries from them.

However you have a point about individual args of constructorArgs, or even values of array properties or fields. But frankly, at this point, a user who is changing the content of an array passed to CustomAttributeInfo is just looking for trouble... so I'm tempted to leave it alone.

blairconrad · 2016-10-23T19:49:44Z

we just create dictionaries from them

Which is a way to store them, no? In dictionaries!

dict.Add(members[i].Name, values[i]);

shoves the element of the propertiesValues or fieldValues right in the dictionary, for later equality-checking. If the element is an array, it can be mutated.

I do agree that anyone changing the hypothetical arrays after the fact is looking for trouble. Then again, when they find it, they'll probably come back with questions and complaints! 😉
Thanks for considering my comment.

thomaslevesque · 2016-10-23T19:57:47Z

shoves the element of the propertiesValues or fieldValues right in the dictionary, for later equality-checking. If the element is an array, it can be mutated.

Ah, I thought you were talking about propertiesValues and fieldValues themselves , not the arrays they might contain...

I do agree that anyone changing the hypothetical arrays after the fact is looking for trouble. Then again, when they find it, they'll probably come back with questions and complaints! 😉

True. @jonorossi, what do you think? Should the code account for that by deep-cloning the arrays? (actually, only nested arrays need to be cloned, since all other types allowed as attribute arguments are immutable)

jonorossi · 2016-10-24T01:53:38Z

Should the code account for that by deep-cloning the arrays? (actually, only nested arrays need to be cloned, since all other types allowed as attribute arguments are immutable)

CustomAttributeBuilder copies the ctor args (because it holds on to them for some reason) but doesn't clone any constructor argument that is an array so therefore CustomAttributeBuilder also isn't fully immutable (however m_blob is). It doesn't copy any arrays in property/field values because they are written into the byte array by the end of the constructor call and no reference is stored.

To avoid wasted allocations I think we should leave the code as is, and just add an XML comment to the class indicating that any arrays passed to the class are now owned by that class and should not be modified. I agree this is an edge case and I'm happy to direct defect reports to an XML comment.

jonorossi · 2016-10-24T02:01:56Z

let me know if you want me to squash some or all the commits before the merge. I don't know how you usually handle this in Castle.Core

I do usually like history in PRs squished if they aren't commits that align to nice clean patches (I'm old school, I like clean patch sets 😉 ), but have got soft over the years as just getting a complete PR seems to be a stretch lately. Many thanks for the work here and being so responsive.

thomaslevesque · 2016-10-24T14:29:15Z

I don't mind squashing into clean commits, that's what we usually do in FakeItEasy.

I squashed the changes down to 4 commits.

jonorossi · 2016-10-26T14:28:08Z

@thomaslevesque just wanted to make sure you didn't miss my reply to your question. Were you going to add that XML comment?

thomaslevesque · 2016-10-26T15:53:56Z

@thomaslevesque just wanted to make sure you didn't miss my reply to your question. Were you going to add that XML comment?

Actually, I did miss it... I'll add the comment.

thomaslevesque · 2016-10-26T19:10:53Z

I added the XML comment in a fixup commit; I'll squash it after you approve it.

jonorossi · 2016-10-27T05:41:44Z

I added the XML comment in a fixup commit; I'll squash it after you approve it.

Exactly what I was expecting, just without the spelling mistake 😉 :

Arrays passed to this class as constructor arguments or property of field values become owned by this class.

to implement proper equality comparison

and related code (AttributeDisassembler)

thomaslevesque · 2016-10-27T07:56:36Z

Exactly what I was expecting, just without the spelling mistake 😉 :

Oops! Fixed and squashed

jonorossi · 2016-10-27T08:29:58Z

Many thanks @thomaslevesque, I'm ready to merge.

blairconrad · 2016-10-27T08:38:20Z

Thanks, @thomaslevesque and @jonorossi!

thomaslevesque · 2016-10-27T08:57:25Z

Thanks for the merge!

* Fix compilation error > MockRepository.cs(213, 17): [CS0618] 'ProxyGenerationOptions.AttributesToAddToGeneratedTypes' is obsolete: 'This property is obsolete and will be removed in future versions. Use AdditionalAttributes property instead. You can use AttributeUtil class to simplify creating CustomAttributeBuilder instances for common cases.' See details: * https://github.com/castleproject/Core/blob/master/CHANGELOG.md#400-beta002-2016-10-28 * castleproject/Core#219

jonorossi added this to the v4.0 milestone Oct 20, 2016

blairconrad reviewed Oct 21, 2016

View reviewed changes

jonorossi mentioned this pull request Oct 21, 2016

File version set to 0.0.0 #214

Closed

thomaslevesque force-pushed the fix-attribute-comparison branch from 5d034e6 to fd5c39a Compare October 21, 2016 21:52

blairconrad suggested changes Oct 22, 2016

View reviewed changes

thomaslevesque force-pushed the fix-attribute-comparison branch from 4dfda22 to a788e16 Compare October 22, 2016 18:19

thomaslevesque changed the title ~~[WIP] Fix attribute comparison for ProxyGenerator cache~~ Fix attribute comparison for ProxyGenerator cache Oct 22, 2016

blairconrad approved these changes Oct 23, 2016

View reviewed changes

jonorossi requested changes Oct 23, 2016

View reviewed changes

thomaslevesque force-pushed the fix-attribute-comparison branch from 15e123a to 8d07b6c Compare October 23, 2016 19:04

jonorossi approved these changes Oct 24, 2016

View reviewed changes

thomaslevesque force-pushed the fix-attribute-comparison branch from 8d07b6c to e67a530 Compare October 24, 2016 14:28

thomaslevesque added 4 commits October 27, 2016 09:55

Replace CustomAttributeBuilder with CustomAttributeInfo

9c16707

to implement proper equality comparison

Add unit tests for CustomAttributeInfo

85665e8

Remove obsolete AttributesToAddToGeneratedTypes property

692d82f

and related code (AttributeDisassembler)

Update CHANGELOG.md

cae3f9f

thomaslevesque force-pushed the fix-attribute-comparison branch from 362f099 to cae3f9f Compare October 27, 2016 07:56

jonorossi merged commit 8fdcf6a into castleproject:master Oct 27, 2016

thomaslevesque deleted the fix-attribute-comparison branch October 27, 2016 08:57

thomaslevesque mentioned this pull request Oct 27, 2016

Adding additional attributes to a fake of an already faked type fails silently FakeItEasy/FakeItEasy#437

Closed

thomaslevesque mentioned this pull request Jan 5, 2019

Extend the set of expressions supported by CustomAttributeInfo.FromExpression #426

Closed

Fix attribute comparison for ProxyGenerator cache #219

Fix attribute comparison for ProxyGenerator cache #219

Conversation

thomaslevesque commented Oct 13, 2016

jonorossi commented Oct 20, 2016

thomaslevesque commented Oct 20, 2016

thomaslevesque commented Oct 20, 2016

blairconrad commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonorossi commented Oct 21, 2016

jonorossi commented Oct 21, 2016

thomaslevesque commented Oct 21, 2016

blairconrad commented Oct 21, 2016

jonorossi commented Oct 21, 2016

thomaslevesque commented Oct 21, 2016

jonorossi commented Oct 22, 2016

adamralph commented Oct 22, 2016

jonorossi commented Oct 22, 2016

blairconrad commented Oct 22, 2016

thomaslevesque commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaslevesque commented Oct 22, 2016

blairconrad commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blairconrad Oct 23, 2016 • edited Loading

Choose a reason for hiding this comment

thomaslevesque commented Oct 23, 2016

thomaslevesque commented Oct 23, 2016

thomaslevesque commented Oct 23, 2016

blairconrad commented Oct 23, 2016

thomaslevesque commented Oct 23, 2016 • edited Loading

jonorossi commented Oct 24, 2016

jonorossi commented Oct 24, 2016

thomaslevesque commented Oct 24, 2016

jonorossi commented Oct 26, 2016

thomaslevesque commented Oct 26, 2016

thomaslevesque commented Oct 26, 2016

jonorossi commented Oct 27, 2016

thomaslevesque commented Oct 27, 2016

jonorossi commented Oct 27, 2016

blairconrad commented Oct 27, 2016

thomaslevesque commented Oct 27, 2016

blairconrad Oct 23, 2016 •

edited

Loading

thomaslevesque commented Oct 23, 2016 •

edited

Loading