Refactor change namespace service #31447

genlu · 2018-11-29T20:56:32Z

This completed the separation of ChangeNamespaceService from SyncNamespaceCodeRefactoringProvider.

Most of the logic for checking the availability of the refactoring now moved to ChangeNamespaceService, with only the bits related to sync namespace with folder remained. The service now also supports changing the name of a namespace declaration when there's multiple namespace declared in the same document, as long as the selected one is not nested.

CyrusNajmabadi · 2018-11-29T21:05:37Z

Can you ping me when you'd lke a review? Thanks!

genlu · 2018-11-30T00:22:41Z

@dotnet/roslyn-ide @CyrusNajmabadi Please take a look. Thanks!

genlu · 2018-12-03T18:48:50Z

Ping @CyrusNajmabadi

CyrusNajmabadi · 2018-12-03T18:55:20Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

+            }
+            else
+            {
+                return default;


does this indicate a bug with teh caller?

Possibly, but since this is a service, I believe we have to handle it. (or the pattern is to throw?)

i would throw then and treat this as argument validation. caller should be passing in the right value (which should be documented).

OK, will fix.

Are you done with the reviewing or you just started?

I'm done reviewing for now.. it looks good to me :) Was going to sign off once you made any further changes and i got a chance to look at them.

CyrusNajmabadi · 2018-12-03T18:56:33Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

            ImmutableArray<string> declaredNamespaceParts, 
            ImmutableArray<string> targetNamespaceParts)
        {
            Debug.Assert(!declaredNamespaceParts.IsDefault && !targetNamespaceParts.IsDefault);
+            var container = root.GetAnnotatedNodes(ContainerAnnotation[0]).Single();


this looks weird. What is ContainerANnotation[0]?

Also... it's indexable... but sounds singular...

I have an single item array for ContainerAnnotation instead so we don't have create it repeatedly, because an array is required for both the overload to add annotation (using params) and the method to remove the annotation.

hmm, for some reason I thought the method removes annotation only takes an array... fixed.

CyrusNajmabadi · 2018-12-03T18:58:14Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

                var targetNamespaceDecl = SyntaxFactory.NamespaceDeclaration(
                    name: CreateNameSyntax(targetNamespaceParts, aliasQualifier: null, targetNamespaceParts.Length - 1)
                            .WithAdditionalAnnotations(WarningAnnotation),
                    externs: default, 
                    usings: default,
                    members: compilationUnit.Members);
-                return compilationUnit.WithMembers(new SyntaxList<MemberDeclarationSyntax>(targetNamespaceDecl));
+                return compilationUnit.WithMembers(new SyntaxList<MemberDeclarationSyntax>(targetNamespaceDecl))


consider extracting this if-body to a helper method.

CyrusNajmabadi · 2018-12-03T18:58:30Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

+                    namespaceDecl.WithName(
+                        CreateNameSyntax(targetNamespaceParts, aliasQualifier: null, targetNamespaceParts.Length - 1)
+                        .WithTriviaFrom(namespaceDecl.Name).WithAdditionalAnnotations(WarningAnnotation))
+                        .WithoutAnnotations(ContainerAnnotation));


consider extracing this if-body to a helper method. this method is getting quite large.

CyrusNajmabadi · 2018-12-03T18:59:56Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

+        /// </summary>
+        protected override async Task<SyntaxNode> TryGetApplicableContainerFromSpanAsync(Document document, TextSpan span, CancellationToken cancellationToken)
+        {
+            var compilationUnit = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) as CompilationUnitSyntax;


just cast. 'as' indicates you think it might not be this type and want to check for null. so an 'as' should always have a null-check, otherwise, you shoudl be casting.

CyrusNajmabadi · 2018-12-03T19:00:33Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

-                // first and last member, otherwise, simply attach all to the EOF token.
-                if (members.Count > 0)
+                var hasNamespaceDecl = compilationUnit.DescendantNodes(IsCompilationUnitOrNamespaceDeclaration)
+                    .OfType<NamespaceDeclarationSyntax>().Any();


you're asking if any descendants of the CompilationUnit are themselve a CompilationUnit. Seems odd.

~~No, I'm asking if any descendants is NamespaceDeclaration~~
Ah, I see what you are saying.

CyrusNajmabadi · 2018-12-03T19:01:27Z

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

-                    var lastWithTrivia = last.WithAppendedTrailingTrivia(namespaceClosingTrivia);
-                    members = members.Replace(last, lastWithTrivia);
+                var namespaceDecl = node.AncestorsAndSelf().OfType<NamespaceDeclarationSyntax>().SingleOrDefault();
+                if (namespaceDecl?.Name.GetDiagnostics().All(diag => diag.DefaultSeverity != DiagnosticSeverity.Error) != true)


the ?. and All and != true are jsut super confusing. First, if you don't have a namespace, handle htat. Then just check the diagnostics of the name.

CyrusNajmabadi · 2018-12-03T19:02:21Z

...CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpSyncNamespaceCodeRefactoringProvider.cs


-        private static SyntaxNode GetTriggeringNode(CompilationUnitSyntax compilationUnit, int position)
-        {
+            var compilationUnit = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) as CompilationUnitSyntax;


CyrusNajmabadi · 2018-12-03T19:02:45Z

...CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpSyncNamespaceCodeRefactoringProvider.cs


-        private static SyntaxNode GetTriggeringNode(CompilationUnitSyntax compilationUnit, int position)
-        {
+            var compilationUnit = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) as CompilationUnitSyntax;
            var namespaceDecls = compilationUnit.DescendantNodes(n => n is CompilationUnitSyntax || n is NamespaceDeclarationSyntax)
                .OfType<NamespaceDeclarationSyntax>().ToImmutableArray();


repeat of the CompilationUnit || NamespcaeDecl code. Repeat of hte "find me the namespace decls" code.

Yes, but I don't want to promote this to SyntaxFacts until we have a need for this in VB service, and there's no good place to share it between the refactoring and service. I think it's Ok since it's just a one liner.

and there's no good place to share it between the refactoring and service.

Can tehre not just be a sibling class with helpers that are shared? That's what we generally do elsewhere...

Sure. I just thought it's a overkill for this.

That's fair. I don't care that much either :) Just something i noticed :)

CyrusNajmabadi · 2018-12-03T19:03:24Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+        /// <summary>
+        /// The annotation used to track applicable container in each document to be fixed.
+        /// </summary>
+        protected static SyntaxAnnotation[] ContainerAnnotation { get; } = new[] { new SyntaxAnnotation() };


Should be plural.

Don't have mutable top level arrays. Someone might change it out from underneat you.

CyrusNajmabadi · 2018-12-03T19:03:56Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+
+        protected abstract string GetDeclaredNamespace(SyntaxNode container);
+
+        protected abstract Task<ImmutableArray<(DocumentId id, SyntaxNode container)>> CanChangeNamespaceWorkerAsync(Document document, SyntaxNode container, CancellationToken cancellationToken);


document the return value here.

CyrusNajmabadi · 2018-12-03T19:04:50Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+        public override async Task<bool> CanChangeNamespaceAsync(Document document, SyntaxNode container, CancellationToken cancellationToken)
+        {
+            var applicableContainers = await CanChangeNamespaceWorkerAsync(document, container, cancellationToken).ConfigureAwait(false);
+            return !applicableContainers.IsDefault;


that CanChangeNamespaceWorkerAsync seems to be returning something non-boolean indicates to me the name o hte method is wonky.

Yes, I agree, I will try to think a better name

CyrusNajmabadi · 2018-12-03T19:05:09Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

-            ImmutableArray<DocumentId> documentIds, 
-            string declaredNamespace, 
+            Document document,
+            SyntaxNode container,
            string targetNamespace, 
            CancellationToken cancellationToken)


docuent what 'container' is

There's comment on the interface

CyrusNajmabadi · 2018-12-03T19:05:38Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+                return solution;
+            }
+
+            var containersFromAllDocuments = await CanChangeNamespaceWorkerAsync(document, container, cancellationToken).ConfigureAwait(false);


this method name doesn't match the value it reutrns.

CyrusNajmabadi · 2018-12-03T19:05:54Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+                return solution;
+            }
+
+            var annotatedSolution = await AnnotateContainersAsync(solution, containersFromAllDocuments, cancellationToken).ConfigureAwait(false);


document what you're doing here.

CyrusNajmabadi · 2018-12-03T19:06:21Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs


-            var solutionAfterNamespaceChange = solution;
+            var ids = containersFromAllDocuments.SelectAsArray(pair => pair.id);


what sort of ids are these? if documentIds, call these documentIds.

CyrusNajmabadi · 2018-12-03T19:06:39Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

@@ -123,7 +153,7 @@ await ChangeNamespaceToMatchFoldersAsync(solutionAfterNamespaceChange, id, decla

                var solutionAfterImportsRemoved = await RemoveUnnecessaryImportsAsync(
                    solutionAfterFirstMerge,
-                    documentIds,
+                    containersFromAllDocuments.SelectAsArray(pair => pair.id),


did you already do this above?

CyrusNajmabadi · 2018-12-03T19:07:27Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+                foreach (var document in documents)
+                {
+                    var container = await TryGetApplicableContainerFromSpanAsync(document, span, cancellationToken).ConfigureAwait(false);
+                    containers.Add((document.Id, container));


you did a try-cal, but didn't check the eturn value. is that intentional?

The if-else below is used as check, should have moved the containers.Add after it.

CyrusNajmabadi · 2018-12-03T19:08:24Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+                    }
+                }
+
+                return spanForContainers.Count == 1 ? containers.ToImmutable() : default;


wait... you only return the container if you had one thing in it... so why is this method returning a list? WHy is it not just returning the container if you found one, or null if you found 0/many?

I'm returning multiple containers if they all have the same span

CyrusNajmabadi · 2018-12-03T19:09:03Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

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


newline above this.

CyrusNajmabadi · 2018-12-03T19:11:45Z

...res/VisualBasic/Portable/CodeRefactorings/SyncNamespace/VisualBasicChangeNamespaceService.vb

@@ -36,17 +38,32 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ChangeNamespace
            End If
        End Function

-        ' This is only reachable when called from a VB refacoring provider, which is not implemented yet.
+        ' TODO: Implement the service for VB


if we haven't done this feature for VB, why do we need this service at all? Why not just remove it entirely?

Need to call TryGetReplacementReferenceSyntax, so we can fix references in VB.

The implementation is coming soon, since we'll need VB implementation of this service for #30896

CyrusNajmabadi · 2018-12-03T19:12:48Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs

+        ///   
+        /// Returns <see langword="true"/> only when all the requirements above are met.
+        /// </summary>
+        Task<bool> CanChangeNamespaceAsync(Document document, SyntaxNode container, CancellationToken cancellationToken);


is this also checking that hte namespace name doesn't match the file name? or is that done at a different level?

Why namespace would match file name?

Basically, i'm trying to figure htis out. Say i have "namespace A.B.C { }" and i'm in a file called A\B\C\Foo.cs would this return 'true' (because it does'nt care about if the namespace is already properly named based on the document), or would it return 'false' (because the namespace is already correct based on the doc.

I see. No, this service doesn't care, it will return ``true`. That's the logic left behind in the SyncNamespace refactoring. All this requires to change a namespace name is that the declaration isn't nested.

Thanks. That's probably worth calling out in the doc comment IMO.

Not sure about this. This service has nothing to do with file name and there's no mention about it in the code. Adding a doc comment here seems odd, especially for people who don't know how this service is evolved from the sync namespace refactoring.

FWIW, as someone reading this, it was the first question that came to mind. i.e. "here's a service that will change a namespace to match something... so... will it do anything if the namespace already matches the document i gave it?" I don't think it hurts to have remarks to help clarify things where there are multiple reasonable interpretations that can be made". Up to you.

Sure, if you think that would help. (Maybe it's me who's biased because I have been working on this and know the difference?)

CyrusNajmabadi · 2018-12-03T19:15:39Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs

+        /// Change namespace for given <paramref name="container"/> to the name specified by <paramref name="targetNamespace"/>.
+        /// Everything declared in the <paramref name="container"/> will be moved to the new namespace. 
+        /// Change will only be made if <see cref="CanChangeNamespaceAsync"/> returns <see langword="true"/> and <paramref name="targetNamespace"/>
+        /// is a valid name for namespace (we use "" to specify global namespace).


end hte sentence after 'for namespace'. Next sentence: Use "" for <paramref targetNamespace> to specify the global namespace.

CyrusNajmabadi · 2018-12-03T19:16:26Z

...Core/Portable/CodeRefactorings/SyncNamespace/AbstractSyncNamespaceCodeRefactoringProvider.cs

-            }
-            return false;
-        }
-
        private static string ChangeNamespaceActionTitle(State state)
            => state.TargetNamespace.Length == 0
                ? FeaturesResources.Change_to_global_namespace
                : string.Format(FeaturesResources.Change_namespace_to_0, state.TargetNamespace);


is this called from multiple locations? if not, just inline.

CyrusNajmabadi · 2018-12-03T19:16:47Z

...Core/Portable/CodeRefactorings/SyncNamespace/AbstractSyncNamespaceCodeRefactoringProvider.cs

                var service = document.GetLanguageService<IChangeNamespaceService>();
-                var solutionChangeAction = new SolutionChangeAction(ChangeNamespaceActionTitle(state), token => service.ChangeNamespaceAsync(state.Solution, state.DocumentIds, state.DeclaredNamespace, state.TargetNamespace, token));
+                var solutionChangeAction = new SolutionChangeAction(


you need a subclass of this type for our telemetry to work.

Hmm, so title alone isn't enough? @heejaechang

CyrusNajmabadi · 2018-12-03T19:19:04Z

...ortable/CodeRefactorings/SyncNamespace/AbstractSyncNamespaceCodeRefactoringProvider.State.cs

+            /// - If <paramref name="relativeTo"/> is "A.B" and <paramref name="namespace"/> is also "A.B", then
+            /// the relative namespace is "".
+            /// - If <paramref name="relativeTo"/> is "" then the relative namespace us <paramref name="namespace"/>.
+            /// </summary>


Specifythat 'null' is returned if there is no relation here.

genlu · 2018-12-04T22:19:16Z

@CyrusNajmabadi Please take another look. Thanks!
@ryzngard could you please review this too? You are going to the the first one to use this service ;)

genlu · 2018-12-05T00:07:46Z

@jinujoseph @vatsalyaagrawal for approval

genlu · 2018-12-06T23:09:36Z

Ping... @CyrusNajmabadi @dotnet/roslyn-ide

CyrusNajmabadi · 2018-12-07T00:10:05Z

I'll be able to get to this tonight. thanks!

genlu · 2018-12-10T19:24:33Z

@CyrusNajmabadi Pushed the change that make it throw on invalid arguments. Please take a look.

ryzngard · 2018-12-10T20:09:35Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+
+            var documents = ids.SelectAsArray(id => solution.GetDocument(id));
+            var containers = ArrayBuilder<(DocumentId, SyntaxNode)>.GetInstance(ids.Length);
+            var spanForContainers = PooledHashSet<TextSpan>.GetInstance();


Since we only care if a single TextSpan exists, why not use a nullable type here and just check for that?

This is implemented this way so we can catch the case where the container nodes found in multiple documents have slightly different spans due to different target frameworks (e.g. maybe caused by #if, etc.), or even completely different kind of container node (although very unlikely)

ryzngard · 2018-12-10T20:28:32Z

...ortable/CodeRefactorings/SyncNamespace/AbstractSyncNamespaceCodeRefactoringProvider.State.cs

+            /// the relative namespace is "".
+            /// - If <paramref name="relativeTo"/> is "" then the relative namespace us <paramref name="namespace"/>.
+            /// </summary>
+            private static string GetRelativeNamespace(string relativeTo, string @namespace, ISyntaxFactsService syntaxFacts)


Should this just be an extension method on ISyntaxFactsService? It seems easily testable, well contained, and reusable.

It doesn't really morally feel like a snytax fact...

It's actually unclear why this would need ISyntaxFactsService in teh first place. If this is just about case-sensitivity, then that feels like it should just be a boolean that is passed in IMO.

I feel this is doing something useful only in a very limited scope (i.e. this service), don't want to promote it until there's a need for it.

I see. What's the benefit of using the ISyntaxFactsService.StringComparer for cases of equality? It looks like it just uses StringComparer.Ordinal . Does the comparison need to align with ISyntaxFactsService?

In general, when you can use ISyntaxFactsService, you do that to avoid duplicate code. Additionally in this case, it is used because VB has different rules: when the refactoring was implemented, we still had VB in mind, even though we later decided not to provide this for VB, the class hierarchy remained the same, just in case we change our mind in the future.

ryzngard · 2018-12-11T19:29:53Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs

+        /// 
+        /// An <see cref="System.ArgumentException"/> will be thrown if:
+        /// 1. <paramref name="container"/> is not a namespace declaration or a compilation unit node.
+        /// 2. <paramref name="targetNamespace"/> is null or an contain invalid character.


nit: "contains an"

ryzngard

Still open question about string comparison, but everything else looks good

genlu · 2018-12-12T20:16:52Z

@jinujoseph for approval

genlu added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 29, 2018

genlu requested a review from a team as a code owner November 29, 2018 20:56

Refactor change namespace service

62058af

genlu force-pushed the refactoringService branch from 40d43ea to 62058af Compare November 29, 2018 21:02

genlu mentioned this pull request Nov 29, 2018

Remaining issues for "sync namespace" refactoring #31253

Open

15 tasks

Some minor changes

0f9a000

genlu removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 30, 2018

jinujoseph added the Area-IDE label Dec 3, 2018

jinujoseph modified the milestones: 16.0, 16.0.P2 Dec 3, 2018

CyrusNajmabadi reviewed Dec 3, 2018

View reviewed changes

Address review comments

e43a1b4

Throw on invalid argumentsto IChangeNamespaceService

9538103

ryzngard reviewed Dec 10, 2018

View reviewed changes

genlu added 2 commits December 10, 2018 14:36

Merge remote-tracking branch 'dotnet/master' into refactoringService

702fe9f

Add some comment to IChangeNamespaceService for clarification

3734404

ryzngard reviewed Dec 11, 2018

View reviewed changes

ryzngard approved these changes Dec 11, 2018

View reviewed changes

Fix typo

a571869

jinujoseph added the Approved to merge label Dec 13, 2018

genlu merged commit fb53ad2 into dotnet:master Dec 13, 2018

genlu deleted the refactoringService branch December 13, 2018 02:06


		protected abstract string GetDeclaredNamespace(SyntaxNode container);

		protected abstract Task<ImmutableArray<(DocumentId id, SyntaxNode container)>> CanChangeNamespaceWorkerAsync(Document document, SyntaxNode container, CancellationToken cancellationToken);


		var solutionAfterNamespaceChange = solution;
		var ids = containersFromAllDocuments.SelectAsArray(pair => pair.id);

Refactor change namespace service #31447

Refactor change namespace service #31447

Conversation

genlu commented Nov 29, 2018 • edited Loading

CyrusNajmabadi commented Nov 29, 2018

genlu commented Nov 30, 2018

genlu commented Dec 3, 2018

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

CyrusNajmabadi Dec 8, 2018 • edited Loading

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

Choose a reason for hiding this comment

genlu Dec 4, 2018 • edited Loading

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

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

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

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

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

genlu commented Dec 4, 2018 • edited Loading

genlu commented Dec 5, 2018

genlu commented Dec 6, 2018

CyrusNajmabadi commented Dec 7, 2018

genlu commented Dec 10, 2018

genlu commented Nov 29, 2018 •

edited

Loading

CyrusNajmabadi Dec 8, 2018 •

edited

Loading

genlu Dec 4, 2018 •

edited

Loading

genlu commented Dec 4, 2018 •

edited

Loading