Make types in Microsoft.AspNetCore.Mvc.Internal namespace internal #8646

pranavkm · 2018-10-25T23:51:37Z

No description provided.

khellang · 2018-10-25T23:57:31Z

Noooo. Why?

JamesNK · 2018-10-26T02:11:27Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerFactoryProvider.cs

-            IEnumerable<IControllerPropertyActivator> propertyActivators
-#pragma warning restore PUB0001
-            )
+            IEnumerable<IControllerPropertyActivator> propertyActivators)


Good 3.0 change 👍

Are there no PUB0001 warnings after this change?

At least not in Mvc.Core

Is @pakrym analyzer on the repo?

Yup, that's unchanged

JamesNK · 2018-10-26T02:20:32Z

test/Microsoft.AspNetCore.Mvc.Core.Test/DependencyInjection/ApiBehaviorOptionsSetupTest.cs

 {
+    using Options = Options.Options;


hmmm? Why here instead of the top of the file with other usings?

JamesNK · 2018-10-26T02:21:20Z

test/Microsoft.AspNetCore.Mvc.Core.TestCommon/Microsoft.AspNetCore.Mvc.Core.TestCommon.csproj

@@ -15,4 +15,8 @@
    <PackageReference Include="Microsoft.Extensions.WebEncoders" Version="$(MicrosoftExtensionsWebEncodersPackageVersion)" />
  </ItemGroup>

+  <ItemGroup>
+    <Folder Include="Properties\" />


Intentional?

JamesNK · 2018-10-26T02:25:00Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultOrderTest.cs

@@ -26,19 +24,19 @@ public DefaultOrderTest(MvcTestFixture<BasicWebSite.Startup> fixture)
        public HttpClient Client { get; }

        [Theory]
-        [InlineData(typeof(IActionDescriptorProvider), typeof(ControllerActionDescriptorProvider), -1000)]
+        [InlineData(typeof(IActionDescriptorProvider), "Microsoft.AspNetCore.Mvc.ApplicationModels.ControllerActionDescriptorProvider", -1000)]


What are the team's thoughts on InternalsVisibleTo to test projects? Personally I don't have a problem with them, and would rather them over using string for type comparisons in tests.

@JamesNK this case is a bit different, unit test projects see all the internals but functional tests do not.

This specific test is kinda awful - it's a functional test, for what can be written as a unit test. Additionally, I'm not entirely sure what the benefit of this test is - it verifies if the Order property on these types are set - we rarely test those since they nearly never change: https://github.com/aspnet/Mvc/blob/release/2.2/test/WebSites/BasicWebSite/Controllers/OrderController.cs#L13-L36

I'm all up for internals visible to for testing purposes, better IVT than having a public/pubternal API (If we are not willing to break it)

I'm planning on removing these tests in a follow up.

poke · 2018-10-26T15:33:41Z

I want to second @khellang here: Why is this being done? What’s the problem with public but messaged-as-internal types?

dougbu · 2018-10-27T23:33:32Z

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ControllerActionDescriptorProvider.cs

@@ -95,14 +94,14 @@ public void OnProvidersExecuted(ActionDescriptorProviderContext context)
            }
        }

-        protected internal IEnumerable<ControllerActionDescriptor> GetDescriptors()
+        internal IEnumerable<ControllerActionDescriptor> GetDescriptors()


Nice. Makes it very clear external subclasses are no longer supported. Same for later virtual removals in this PR.

dougbu · 2018-10-27T23:45:34Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultOrderTest.cs

@@ -26,19 +24,19 @@ public DefaultOrderTest(MvcTestFixture<BasicWebSite.Startup> fixture)
        public HttpClient Client { get; }

        [Theory]
-        [InlineData(typeof(IActionDescriptorProvider), typeof(ControllerActionDescriptorProvider), -1000)]
+        [InlineData(typeof(IActionDescriptorProvider), "Microsoft.AspNetCore.Mvc.ApplicationModels.ControllerActionDescriptorProvider", -1000)]


@JamesNK this case is a bit different, unit test projects see all the internals but functional tests do not.

dougbu · 2018-10-27T23:47:43Z

src/Microsoft.AspNetCore.Mvc.Core/ActionConstraints/ActionConstraintCache.cs

 {
-    public class ActionConstraintCache
+    internal sealed class ActionConstraintCache


Adding sealed probably doesn't hurt. But, it doesn't help much with internal types and is added randomly in this PR. Suggest reverting all of these additions.

I sort of did this to identify virtual methods that were implemented on previously pub internal types. Seems pretty harmless, but if you feel strongly about it I can remove it.

If its just one find and replace, I would change it. That way in the future people will not wonder why they are internal sealed or think that this is an actual code pattern that we follow

Agree w/ @javiercn

dougbu · 2018-10-27T23:48:28Z

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/DefaultApplicationModelProvider.cs

@@ -475,7 +474,7 @@ protected virtual ParameterModel CreateParameterModel(ParameterInfo parameterInf
            return parameterModel;
        }

-        private IList<SelectorModel> CreateSelectors(IList<object> attributes)
+        internal IList<SelectorModel> CreateSelectors(IList<object> attributes)


Huh? Why is this changing?

pranavkm · 2018-11-05T21:37:25Z

src/Microsoft.AspNetCore.Mvc.Core/ActionConstraints/ActionConstraintCache.cs

 {
-    public class ActionConstraintCache
+    internal sealed class ActionConstraintCache


I sort of did this to identify virtual methods that were implemented on previously pub internal types. Seems pretty harmless, but if you feel strongly about it I can remove it.

pranavkm · 2018-11-05T21:42:31Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerFactoryProvider.cs


 namespace Microsoft.AspNetCore.Mvc.Controllers
 {
-    public class ControllerFactoryProvider : IControllerFactoryProvider
+    internal class ControllerFactoryProvider : IControllerFactoryProvider


I'm a bit undecided on some types that previously exposed pub-internal types. On one hand,

we could make the pub-internal type public and that's less change.

On the other hand, things like IControllerPropertyActivator are an implementation detail and I'm not super sure if we it needs to be made public. Additionally, our guidance would be to use the interface contract IControllerFactoryProvider rather than the implementation type in user code.

Thoughts \ suggestions?

I prefer we go "internal by default" and that we revisit internal/pub-ternal types 1 by 1 based on scenarios. This has caused us pain in the past (for example limiting our ability to do breaking changes) when we've added virtual methods to pub-ternal types without thinking about them and then having them be a limiting factor in next releases

As long as we're left with nothing in ...Internal namespaces, we don't change anything truly public to internal, and public members exclusively use public types, I think we're fine. There's probably a few things on the margin that we should discuss case-by-case.

And, in this case, internal makes sense

pranavkm · 2018-11-05T21:42:45Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerFactoryProvider.cs

-            IEnumerable<IControllerPropertyActivator> propertyActivators
-#pragma warning restore PUB0001
-            )
+            IEnumerable<IControllerPropertyActivator> propertyActivators)


At least not in Mvc.Core

pranavkm · 2018-11-05T21:43:56Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs


 namespace Microsoft.AspNetCore.Mvc.Controllers
 {
    /// <summary>
    /// <see cref="IControllerActivator"/> that uses type activation to create controllers.
    /// </summary>
-    public class DefaultControllerActivator : IControllerActivator
+    internal sealed class DefaultControllerActivator : IControllerActivator


Same question as DefaultControllerFactory. ITypeActivatorCache is pub-internal in 2.x and feels like infrastructure that doesn't need to be made public.

We should think about how users extend the framework or what story do we want to tell them. We need to strike a balance between overriding a method or telling them to provide their own entire implementation. (Specially for classes with a lot of dependencies or complex implementations). If people end up copying the code of the internal type they automatically opt-out of patches, etc for that area of the code base, and that's not a good story. In the past we had a lot of problems in traditional web api because of these type of things.

We shouldn't make any truly public class internal in this sweep. Err on the side of compatibility. And, if that means ITypeActivatorCache must be public, that's 🆗

pranavkm · 2018-11-05T21:47:21Z

src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaTypeSegmentWithQuality.cs

 {
    /// <summary>
    /// A media type with its associated quality.
    /// </summary>
-    public readonly struct MediaTypeSegmentWithQuality
+    internal readonly struct MediaTypeSegmentWithQuality


@javiercn do you know if we could use https://github.com/aspnet/HttpAbstractions/blob/master/src/Microsoft.Net.Http.Headers/MediaTypeHeaderValue.cs instead of doing our own parsing?

There was an effort to do this by @jkotalik but I'm not sure where it ended. There were some differences in behavior I believe, as we are more "lax" parsing things in order to maintain compatibility with bad behaving clients, all this done for historical reasons where the knowledge has been lost in time.

I think we should consider deprecating the MediaType class in 2.2 and replacing it with MediaTypeHeaderValue (MTHV). I was working on making MediaType a wrapper class around MTHV, but there were a good number of functional differences and the effort was abandoned.

2.2's done, so deprecating it isn't an option. But we could make an announcement and deprecate the type in 3.0

Then don't we need to wait until 4.x to remove it?

We can probably completely replace it as it’s a major version and I don’t think this type will have a lot of external usage. As long as we preserve the same set of allowed inputs it should be ok

@Eilon is this something we can consider making Obsolete in 2.2?

pranavkm · 2018-11-05T21:49:08Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormValueProvider.cs

@@ -46,9 +45,7 @@ public class FormValueProvider : BindingSourceValueProvider, IEnumerableValuePro

        public CultureInfo Culture => _culture;

-#pragma warning disable PUB0001 // Pubternal type in public API
        protected PrefixContainer PrefixContainer


PrefixContainer seems like a fairly useful \ well-designed type and I was planning on making it public as part of this change. Concerns?

pranavkm · 2018-11-05T21:51:49Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ObjectModelValidator.cs

@@ -101,9 +100,7 @@ public abstract class ObjectModelValidator : IObjectModelValidator
        public abstract ValidationVisitor GetValidationVisitor(
            ActionContext actionContext,
            IModelValidatorProvider validatorProvider,
-#pragma warning disable PUB0001 // Pubternal type in public API
            ValidatorCache validatorCache,


This API is used by FluentValidation and I didn't want to do too much refactoring here to avoid churn in their library. ValidatorCache is made public in this change, but it's a very infrastructural type that libraries shouldn't need to interact with

pranavkm · 2018-11-05T21:59:45Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponents/DefaultViewComponentActivator.cs

@@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewComponents
    /// <see cref="ViewComponentContext"/> to a public property of a view component marked
    /// with <see cref="ViewComponentContextAttribute"/>.
    /// </remarks>
-    public class DefaultViewComponentActivator : IViewComponentActivator
+    internal sealed class DefaultViewComponentActivator : IViewComponentActivator


Suggestions?

This has the same issue as other public types that exposed pub-internal types. I've listed individual types that changed as a result since it warrants looking at them on a case by case basis

Though I prefer we don't make public types internal, this change seems fine

pranavkm · 2018-11-05T22:00:00Z

...Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultValidationHtmlAttributeProvider.cs

@@ -14,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures
    /// <summary>
    /// Default implementation of <see cref="ValidationHtmlAttributeProvider"/>.
    /// </summary>
-    public class DefaultValidationHtmlAttributeProvider : ValidationHtmlAttributeProvider
+    internal sealed class DefaultValidationHtmlAttributeProvider : ValidationHtmlAttributeProvider


Suggestions?

I suggest this should be public since it's difficult to extend the validation system without it. If that means we expose ClientValidatorCache or a new IClientValidatorCache interface, fine.

pranavkm · 2018-11-05T22:03:51Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultOrderTest.cs

@@ -26,19 +24,19 @@ public DefaultOrderTest(MvcTestFixture<BasicWebSite.Startup> fixture)
        public HttpClient Client { get; }

        [Theory]
-        [InlineData(typeof(IActionDescriptorProvider), typeof(ControllerActionDescriptorProvider), -1000)]
+        [InlineData(typeof(IActionDescriptorProvider), "Microsoft.AspNetCore.Mvc.ApplicationModels.ControllerActionDescriptorProvider", -1000)]


This specific test is kinda awful - it's a functional test, for what can be written as a unit test. Additionally, I'm not entirely sure what the benefit of this test is - it verifies if the Order property on these types are set - we rarely test those since they nearly never change: https://github.com/aspnet/Mvc/blob/release/2.2/test/WebSites/BasicWebSite/Controllers/OrderController.cs#L13-L36

javiercn · 2018-11-05T23:51:46Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs

            _propertyActivators = propertyActivators.ToArray();
        }

        /// <summary>
        /// The <see cref="IControllerActivator"/> used to create a controller.
        /// </summary>
-        protected IControllerActivator ControllerActivator => _controllerActivator;
+        private IControllerActivator ControllerActivator { get; }


Is there any reason to move these things into props and make them explicitly private? I would have left the private readonly var instead. We don't do private properties anywhere AFAIK

dougbu · 2018-11-06T17:45:45Z

src/Microsoft.AspNetCore.Mvc.Core/ActionConstraints/ActionConstraintCache.cs

 {
-    public class ActionConstraintCache
+    internal sealed class ActionConstraintCache


Agree w/ @javiercn

dougbu · 2018-11-06T17:49:39Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerFactoryProvider.cs


 namespace Microsoft.AspNetCore.Mvc.Controllers
 {
-    public class ControllerFactoryProvider : IControllerFactoryProvider
+    internal class ControllerFactoryProvider : IControllerFactoryProvider


As long as we're left with nothing in ...Internal namespaces, we don't change anything truly public to internal, and public members exclusively use public types, I think we're fine. There's probably a few things on the margin that we should discuss case-by-case.

dougbu · 2018-11-06T17:51:06Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerFactoryProvider.cs


 namespace Microsoft.AspNetCore.Mvc.Controllers
 {
-    public class ControllerFactoryProvider : IControllerFactoryProvider
+    internal class ControllerFactoryProvider : IControllerFactoryProvider


And, in this case, internal makes sense

dougbu · 2018-11-06T17:53:01Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs


 namespace Microsoft.AspNetCore.Mvc.Controllers
 {
    /// <summary>
    /// <see cref="IControllerActivator"/> that uses type activation to create controllers.
    /// </summary>
-    public class DefaultControllerActivator : IControllerActivator
+    internal sealed class DefaultControllerActivator : IControllerActivator


We shouldn't make any truly public class internal in this sweep. Err on the side of compatibility. And, if that means ITypeActivatorCache must be public, that's 🆗

dougbu · 2018-11-06T17:54:48Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs


 namespace Microsoft.AspNetCore.Mvc.Controllers
 {
    /// <summary>
    /// Default implementation for <see cref="IControllerFactory"/>.
    /// </summary>
-    public class DefaultControllerFactory : IControllerFactory
+    internal sealed class DefaultControllerFactory : IControllerFactory


This looks like another change in the wrong direction. Again, err on the side of compatibility

src/Microsoft.AspNetCore.Mvc.Razor/Internal/DefaultTagHelperActivator.cs

dougbu · 2018-11-06T19:13:46Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs

@@ -21,7 +21,7 @@

 namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
 {
-    public class PageActionInvoker : ResourceInvoker, IActionInvoker
+    internal class PageActionInvoker : ResourceInvoker, IActionInvoker


Again, empty out the .Internal namespaces

Focusing on Mvc.Core for now. Will do this in follow up

dougbu · 2018-11-06T19:16:25Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponents/DefaultViewComponentActivator.cs

@@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewComponents
    /// <see cref="ViewComponentContext"/> to a public property of a view component marked
    /// with <see cref="ViewComponentContextAttribute"/>.
    /// </remarks>
-    public class DefaultViewComponentActivator : IViewComponentActivator
+    internal sealed class DefaultViewComponentActivator : IViewComponentActivator


Though I prefer we don't make public types internal, this change seems fine

dougbu · 2018-11-06T19:17:55Z

...Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultValidationHtmlAttributeProvider.cs

@@ -14,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures
    /// <summary>
    /// Default implementation of <see cref="ValidationHtmlAttributeProvider"/>.
    /// </summary>
-    public class DefaultValidationHtmlAttributeProvider : ValidationHtmlAttributeProvider
+    internal sealed class DefaultValidationHtmlAttributeProvider : ValidationHtmlAttributeProvider


I suggest this should be public since it's difficult to extend the validation system without it. If that means we expose ClientValidatorCache or a new IClientValidatorCache interface, fine.

dougbu · 2018-11-06T19:20:19Z

test/WebSites/BasicWebSite/Controllers/OrderController.cs

@@ -19,7 +19,7 @@ public int GetServiceOrder(string serviceType, string actualType)
            var services = (IEnumerable<object>)HttpContext?.RequestServices.GetService(queryType);
            foreach (var service in services)
            {
-                if (actualType != null && service.GetType().AssemblyQualifiedName == actualType)
+                if (actualType != null && service.GetType().FullName == actualType)


Unrelated bug?

I don't want to pass the full name with version along. Anyway, this test should go away soon.

javiercn · 2018-11-06T19:27:37Z

test/Microsoft.AspNetCore.Mvc.Core.TestCommon/CommonResourceInvokerTest.cs

@@ -29,7 +29,7 @@ public abstract class CommonResourceInvokerTest
            return CreateInvoker(new IFilterMetadata[] { filter }, exception, result, valueProviderFactories);
        }

-        protected abstract ResourceInvoker CreateInvoker(
+        protected abstract IActionInvoker CreateInvoker(


Why is this changing, is ResourceInvoker internal?

pranavkm · 2018-11-06T23:39:37Z

🆙 📅

pranavkm · 2018-11-08T17:18:22Z

Plan on merging this today. Let me know if there's any additional feedback.

dougbu

Looks good overall. But, I have a few smaller comments and questions…

dougbu · 2018-11-08T19:17:40Z

src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaTypeSegmentWithQuality.cs

@@ -3,7 +3,7 @@

 using Microsoft.Extensions.Primitives;

-namespace Microsoft.AspNetCore.Mvc.Formatters.Internal
+namespace Microsoft.AspNetCore.Mvc.Formatters


To confirm: For now at least, we're making this truly public?

Yeah. Tracking this via #8694. I'll set up a design review for this once I'm done with the remainder of assemblies.

dougbu · 2018-11-08T19:19:58Z

src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/NonDisposableStream.cs

@@ -34,7 +34,7 @@ public NonDisposableStream(Stream innerStream)
        /// <summary>
        /// The inner stream this object delegates to.
        /// </summary>
-        protected Stream InnerStream => _innerStream;
+        private Stream InnerStream => _innerStream;


Minor but this property no longer looks useful. Just use _innerStream.

dougbu · 2018-11-08T19:23:29Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationStack.cs


-namespace Microsoft.AspNetCore.Mvc.ModelBinding.Internal
+namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
 {
    public class ValidationStack


What drives this one becoming truly public?

It was exposed as a protected property on ValidationVisitor. My guess is that we should be safe to make the property private and the type internal, but let's sort this as part of #8694

src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsHttpMethodActionConstraint.cs

dougbu · 2018-11-08T19:28:34Z

test/Microsoft.AspNetCore.Mvc.Core.Test/DependencyInjection/ApiBehaviorOptionsSetupTest.cs

 using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.Extensions.Logging.Abstractions;
-using Microsoft.Extensions.Options;


Why remove this using? Seems to make the Options.Create(...) calls messier.

Since the type is is Microsoft.Extension.DependencyInjection, doing Options. inside refers to the namespace and not the type. Kinda have to namespace qualify Options

tillig · 2018-11-09T02:04:21Z

src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionSelector.cs

@@ -36,7 +36,7 @@ public interface IActionSelector
        /// <param name="context">The <see cref="RouteContext"/> associated with the current request.</param>
        /// <param name="candidates">The set of <see cref="ActionDescriptor"/> candidates.</param>
        /// <returns>The best <see cref="ActionDescriptor"/> candidate for the current request or <c>null</c>.</returns>
-        /// <exception cref="Internal.AmbiguousActionException">
+        /// <exception cref="AmbiguousActionException">


AmbiguousActionException just got made internal but this interface is public.

pranavkm force-pushed the prkrishn/no-internal branch from 2184955 to 1fa40b8 Compare October 25, 2018 23:57

pranavkm force-pushed the prkrishn/no-internal branch from 1fa40b8 to 8e61598 Compare October 25, 2018 23:57

pranavkm changed the title ~~Make .Internal namespaces types to internal types~~ Make types in Microsoft.AspNetCore.Mvc.Internal namespace internal Oct 25, 2018

JamesNK reviewed Oct 26, 2018

View reviewed changes

dougbu suggested changes Oct 27, 2018

View reviewed changes

pranavkm force-pushed the prkrishn/no-internal branch 3 times, most recently from f722a1e to 3871e95 Compare November 5, 2018 21:57

pranavkm commented Nov 5, 2018

View reviewed changes

javiercn reviewed Nov 5, 2018

View reviewed changes

dougbu suggested changes Nov 6, 2018

View reviewed changes

javiercn reviewed Nov 6, 2018

View reviewed changes

pranavkm mentioned this pull request Nov 6, 2018

Review pubinternal types made public in 3.0 #8694

Closed

pranavkm force-pushed the prkrishn/no-internal branch 6 times, most recently from 8cb0c6e to 4eea841 Compare November 6, 2018 23:23

dougbu approved these changes Nov 8, 2018

View reviewed changes

Make types in Microsoft.AspNetCore.Mvc.Internal namespace internal

7cf914f

pranavkm force-pushed the prkrishn/no-internal branch from 4eea841 to 7cf914f Compare November 8, 2018 21:40

pranavkm merged commit 8d66f10 into master Nov 8, 2018

pranavkm deleted the prkrishn/no-internal branch November 8, 2018 22:00

tillig reviewed Nov 9, 2018

View reviewed changes

Make types in Microsoft.AspNetCore.Mvc.Internal namespace internal #8646

Make types in Microsoft.AspNetCore.Mvc.Internal namespace internal #8646

Uh oh!

Conversation

pranavkm commented Oct 25, 2018

Uh oh!

khellang commented Oct 25, 2018

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

Uh oh!

poke commented Oct 26, 2018

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

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!