Add Make Field Readonly analyzer/code fix #19067

Hosch250 · 2017-04-28T03:51:28Z

VSO bug : https://devdiv.visualstudio.com/DevDiv/_workitems/edit/590185

Specs

Only works on private fields (more accessible fields should be replaced with properties anyway)
Quick fix separates any fields declared in a single statement
Fields are split for Fix One and declared in the same order as originally
For Fix All, if all the fields in a declaration can be readonly, they are not split
User-facing option allows user to enable/disable and set the severity for this analyzer

JohanLarsson · 2017-04-28T04:58:04Z

Add a test for when the field is assigned in a lamda defined in the ctor.
Pseudo code:

public class Foo
{
    private int a;

    public Foo()
    {
        this.E += (_, __) => this.a = 5;
    }

    public event EventHandler E;
}

JohanLarsson · 2017-04-28T05:05:49Z

Test for when another instance is assigned in te constructor.
Pseudo code:

public class Foo
{
    private int value;

    public Foo()
    {
        var foo = new Foo();
        foo.value = 5;
    }
}

Remember to check when the field is assigned using object intitailizer style also.

Pilchie · 2017-04-28T20:21:48Z

Tagging @dotnet/roslyn-ide for review.

CyrusNajmabadi · 2017-04-28T20:24:14Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
+        public async Task FieldIsInternal()


any protected or protected-internal tests?

No, I can add those if you like.

@CyrusNajmabadi This would be a place where I would have used a [Theory] instead of [Fact], in order to highlight that the only change in the individual test cases is the accessibility modifier.

@Hosch250 This is just a comment, no action required.

So i'm amenable to theories if the following is true:

The theories are entirely self describing.

The theory only changes one aspect of the test.

The thing i want to avoid (which i've seen in other tests in our codebase) is something like so:

public void Test() { Test($".........{ subst1 } ........ { subst2 } .......... { subst3 } ......... { subst4 } ........."); }

When this happens, the test, and the permutated data become so decoupled as to great diminish the ability to understand what's happening. You lose the forest for the trees as it were.

If the test is really only permuting a single thing, i would have less issue with this sort of approach.

CyrusNajmabadi · 2017-04-28T20:25:11Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+    class MyClass
+    {
+        private int _bar = 0;
+        private readonly int _foo = 0;


i don't like this. if both could be made readonly, the user is going to want the declarations to stay together. only if one coudl not be made readonly should it be split.

OK. I split them to make the fix simpler, and because I don't like fields being declared together anyway. It should be fairly trivial to move some of the code to a helper file and use it in the code fix too.

@CyrusNajmabadi I'm not worried. This is a relatively rare case. A follow-up issue could be added to make the Fix All handle the case differently where possible.

❗️ @Hosch250 The part I definitely don't like here is the reordering of the initializers. When splitting multiple initializers, the order should not change. This is required in order to preserve the semantics of the original statement during the change. Make sure to test all three cases:

private int _changed, _extra;

private int _extra, _changed;

private int _extra1, _changed, _extra2;

In the latter case the result should be this:

private int _extra1; private readonly int _changed; private int _extra2;

OK. I'll change this.

💡 Would be good to have tests for preserving comments when splitting.

CyrusNajmabadi · 2017-04-28T20:31:30Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+        public async Task PassedAsParameter()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"namespace ConsoleApplication1


all the namespaces can be removed from all tests.

CyrusNajmabadi · 2017-04-28T20:31:48Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+{
+    class MyClass
+    {
+        private int [|_foo|];


why can't this be readonly?

Huh, it can be. I must have missed that when I copy/pasted the test.

That one is missing the attribute. No wonder it didn't fail on me.

CyrusNajmabadi · 2017-04-28T20:38:00Z

src/Features/CSharp/Portable/MakeFieldReadonly/CSharpMakeFieldReadonlyDiagnosticAnalyzer.cs

+
+        internal override bool CanBeReadonly(SemanticModel model, SyntaxNode node)
+        {
+            if (node.Parent is AssignmentExpressionSyntax assignmentNode && assignmentNode.Left == node)


We have a helper called IsWrittenTo that you can use for this purpose. It should replace this entire method.

CyrusNajmabadi · 2017-04-28T20:38:58Z

src/Features/CSharp/Portable/MakeFieldReadonly/CSharpMakeFieldReadonlyDiagnosticAnalyzer.cs

+        internal override bool IsMemberOfThisInstance(SyntaxNode node)
+        {
+            // if it is a qualified name, make sure it is `this.name`
+            if (node.Parent is MemberAccessExpressionSyntax memberAccess)


Consider using IOperation and getting the IFieldReference operation for this, and validating it that way.

I can't use IOperation because it doesn't fire on field declarations without assignments.

CyrusNajmabadi · 2017-04-28T20:43:16Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs

+                var model = await document.GetSemanticModelAsync().ConfigureAwait(false);
+                var symbol = (IFieldSymbol)model.GetDeclaredSymbol(declaration);
+
+                var generator = SyntaxGenerator.GetGenerator(document);


generator available off of the 'editor' instance.

CyrusNajmabadi · 2017-04-28T20:43:40Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs

+            }
+        }
+
+        private async void MakeFieldReadonly(Document document, SyntaxEditor editor, SyntaxNode root, TVariableDeclaratorSyntax declaration)


i don't think you use root.

CyrusNajmabadi · 2017-04-28T20:44:07Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs

+            }
+        }
+
+        internal abstract SyntaxNode GetInitializerNode(TVariableDeclaratorSyntax declaration);


internal? can this be protected?

CyrusNajmabadi · 2017-04-28T20:44:40Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs

+        internal abstract SyntaxNode GetInitializerNode(TVariableDeclaratorSyntax declaration);
+        internal abstract int GetVariableDeclaratorCount(TFieldDeclarationSyntax declaration);
+
+        protected override Task FixAllAsync(


why did you need this override?

It is required by the base class.

CyrusNajmabadi · 2017-04-28T20:45:25Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs

+        {
+            var typeSymbol = (ITypeSymbol)context.SemanticModel.GetDeclaredSymbol(context.Node);
+
+            var nonReadonlyFieldMembers = new HashSet<IFieldSymbol>();


pool this set.

@CyrusNajmabadi Would using a List<T> fix this? It doesn't have to be a HashSet<T>.

Check out PooledHashSet :)

CyrusNajmabadi · 2017-04-28T20:46:15Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs

+            var membersCanBeReadonly = nonReadonlyFieldMembers;
+            foreach (var syntaxReference in typeSymbol.DeclaringSyntaxReferences)
+            {
+                var typeNode = syntaxReference.SyntaxTree.GetRoot().FindNode(syntaxReference.Span);


use cancellation token for GEtRoot.

CyrusNajmabadi · 2017-04-28T20:47:19Z

src/Workspaces/Core/Portable/WorkspacesResources.resx

@@ -613,4 +613,7 @@
  <data name="Changing_document_property_is_not_supported" xml:space="preserve">
    <value>Changing document properties is not supported</value>
  </data>
+  <data name="Make_field_readonly" xml:space="preserve">


Should be at feature level, not workspace leve.

OK. Is there any particular reason the Populate Switch message is at workspace level?

sharwell · 2017-04-28T22:24:54Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
+        public async Task FieldIsInternal()


@CyrusNajmabadi This would be a place where I would have used a [Theory] instead of [Fact], in order to highlight that the only change in the individual test cases is the accessibility modifier.

@Hosch250 This is just a comment, no action required.

sharwell · 2017-04-28T22:28:33Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+    class MyClass
+    {
+        private int _bar = 0;
+        private readonly int _foo = 0;


@CyrusNajmabadi I'm not worried. This is a relatively rare case. A follow-up issue could be added to make the Fix All handle the case differently where possible.

❗️ @Hosch250 The part I definitely don't like here is the reordering of the initializers. When splitting multiple initializers, the order should not change. This is required in order to preserve the semantics of the original statement during the change. Make sure to test all three cases:

private int _changed, _extra;

private int _extra, _changed;

private int _extra1, _changed, _extra2;

In the latter case the result should be this:

private int _extra1; private readonly int _changed; private int _extra2;

sharwell · 2017-04-28T22:35:34Z

src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs

+        private int [|_foo|];
+        void Foo()
+        {
+            Bar(ref _foo);


💡 Add a test where this is used only in the constructor (allowed). If not supported, keep the test but file a follow-up issue to support the scenario in the future.

💡 Add matching tests for passing an out variable.

OK. That should work because I do the ctor test first.

CyrusNajmabadi · 2017-04-28T22:53:40Z

This will need a user facing option if we're going to take this :)

Note: i'm not sure if this belongs in Roslyn, or would be better in the Roslyn-analyzers repo. Thoughts @Pilchie ?

Hosch250 · 2017-04-28T22:54:11Z

Don't bother checking my last changes--I broke things, then committed without compiling and accidentally synced instead of waiting until I'd fixed everything.

Hosch250 · 2017-04-28T23:01:05Z

Would you like the user-facing option in this PR, or would you take it in another PR? I think I'll wait until this is finished before adding that either way.

CyrusNajmabadi · 2017-04-28T23:14:48Z

Would you like the user-facing option in this PR,

It would need to be in this PR. We only really want to add features if they can be considered ready for shipping to users. Thanks!

Hosch250 · 2017-04-29T04:40:50Z

@CyrusNajmabadi @sharwell Please check out my latest changes. I followed sharwell's suggestion about splitting the declarations for Fix One and keeping them in the same order, but I do not split them for Fix All if they can all be readonly. When any need to be split, I split them all just to be consistent.

sharwell · 2017-04-29T13:44:23Z

It would need to be in this PR.

@Hosch250 Note that this would fall into a category where it may lie outside your area of expertise, and we wouldn't want to reject a feature that comes with tests and all just because of this. If this is a requirement for acceptance, then we would do one of the following to help you through it:

Find time to help you through the details in a timely manner, or
Create a feature branch in the repository so this could be incorporated without the user-facing option there, and finally merged into master after someone adds the option.

In the next week, we'll have a discussion about the scope of analyzers we want to see here as opposed to separately (e.g. dotnet/roslyn-analyzers, or some other location). For analyzers in those other repositories, the user-facing option is much simpler - it isn't supported so it's not required. 😄

Hosch250 · 2017-04-29T15:47:16Z

@sharwell This would not be outside of my experience--I did it for another feature, but didn't have time to follow it through to completion because college started. It is pretty much just copy/pasting code from other options.

Hosch250 · 2017-04-29T17:02:09Z

@sharwell @CyrusNajmabadi The user-facing option is done.

sharwell · 2017-04-29T17:12:59Z

@Hosch250 Some users may not realize that adding readonly to a field whose type is a mutable value type (struct) can change semantics and possibly break the behavior of an application. Have we taken any steps in the analysis or the code fix (especially the Fix All) to help avoid quietly breaking the behavior in ways that a user is likely to not understand?

For example, this field is only assigned in the constructor, but if you mark it with readonly it will break the behavior of the type.

…ormance

mavasani · 2018-03-23T17:48:07Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs

+            var syntaxFactsService = GetSyntaxFactsService();
+            var root = semanticModel.SyntaxTree.GetRoot(cancellationToken);
+            var candidateTypes = PooledHashSet<(ITypeSymbol, SyntaxNode)>.GetInstance();
+            AddCandidateTypesInCompilationUnit(semanticModel, root, candidateTypes, cancellationToken);


We would not need this method if the analyzer were to be written as a symbol analyzer which registers for SymbolKind.NamedType. We can directly check if the symbol has 1 declaring reference/location to consider it as a candidate.

mavasani · 2018-03-23T17:48:56Z

src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs

+            candidateFields.Free();
+        }
+
+        private void AddCandidateFieldsInType(SemanticModelAnalysisContext context, ITypeSymbol typeSymbol, SyntaxNode typeSyntax, PooledHashSet<IFieldSymbol> candidateFields)


CyrusNajmabadi · 2018-03-23T18:29:58Z

In addition to the restriction to only consider private fields, we will avoid false positives by excluding partial classes from consideration, except where they are nested within a non-partial class.

Hrm. This doesn't seem necessary. As you mentioned "In the short term (target 15.7) we will leverage the same approach used by the Use Auto Property analyzer". UseAutoProp doesn't need to exclude partial classes. It just analyzes the properties it finds in the parts it finds within a SemanticModel. It feels liek the same would be done for MakeFieldReadOnly. We would analyze in practically the same manner.

Both features feel very similar from an implementation perspective. They look at properties or fields in a single part of a partial type at a time. And they also look for things that disallow the conversion from happening. Once hte whole semantic model has been walked, eligible matches are reported.

sharwell · 2018-03-23T18:36:36Z

Hrm. This doesn't seem necessary.

Partial classes are excluded to ensure no diagnostic is reported in the following case:

// A.cs
partial class MyType
{
  private int _value;
}

// B.cs
partial class MyType
{
  private void SetValue(int value) { _value = value; }
}

CyrusNajmabadi · 2018-03-23T18:37:22Z

Note: while UseAutoProperty does necessarily have to examine all parts when deciding on eligibility, it does so batched up at the end. i.e. after finding all the properties htat could be made into an auto-property in a single file, it then goes and batches them, aggregating all their containing types (in case they belonged to different types). It then groups those types by the files their parts are contained in. And it processes each of those files once, examining them in their entirety for things that would make the fix ineligible.

This approach seems like it would fit to a T what we need for MakeFieldReadOnly.

CyrusNajmabadi · 2018-03-23T18:39:33Z

Partial classes are excluded to ensure no diagnostic is reported in the following case:

UseAutoProp has no problem with that scenario. There are two passes. The first pass collects the candidates (in your case _value). The second pass computes eligibility. The second pass would see here that _value was written in a disallowed location and would remove _value from the candidate set.

See:

CSharpUseAutoPropertyAnalyzer.RegisterIneligibleFieldsAction

roslyn/src/EditorFeatures/CSharp/UseAutoProperty/CSharpUseAutoPropertyAnalyzer.cs

Lines 64 to 85 in 5fbba20

    
           protected override void RegisterIneligibleFieldsAction( 
        
               List<AnalysisResult> analysisResults, HashSet<IFieldSymbol> ineligibleFields, 
        
               Compilation compilation, CancellationToken cancellationToken) 
        
           { 
        
               var groups = analysisResults.Select(r => (TypeDeclarationSyntax)r.PropertyDeclaration.Parent) 
        
                                           .Distinct() 
        
                                           .GroupBy(n => n.SyntaxTree); 
        
               foreach (var group in groups) 
        
               { 
        
                   var tree = group.Key; 
        
                   var semanticModel = compilation.GetSemanticModel(tree); 
        
                   foreach (var typeDeclaration in group) 
        
                   { 
        
                       foreach (var argument in typeDeclaration.DescendantNodesAndSelf().OfType<ArgumentSyntax>()) 
        
                       { 
        
                           AnalyzeArgument(semanticModel, argument, ineligibleFields, cancellationToken); 
        
                       } 
        
                   } 
        
               } 
        
           }

sharwell · 2018-03-23T18:41:36Z

@CyrusNajmabadi It's possible we could expand support in the future, but we decided to scope limit the analysis to single-file cases only to increase confidence in the runtime performance of the initial implementation given the relatively short time we'll have to evaluate it before release.

CyrusNajmabadi · 2018-03-23T18:43:26Z

@CyrusNajmabadi It's possible we could expand support in the future, but we decided to scope limit the analysis to single-file cases only to increase confidence in the runtime performance of the initial implementation given the relatively short time we'll have to evaluate it before release.

That's reasonable. I can def accept perf concerns vs the false-positive concern. Could we file an issue on that choice? Perhaps linking to this discussion? I perf tested UseAutoProp out on roslyn, and didn't encounter any issues with this approach (and roslyn uses partial types a lot). But i can understand not wanting to risk things. it would be nice though if this could be added later when proper due diligence can be done! Thanks!

If you'd like, i can also file it.

sharwell · 2018-03-23T19:32:47Z

@CyrusNajmabadi Sure, I already filed an issue to migrate to IOperation (where this problem would not occur) but you could separately call out the improvement to handling partial types in a new issue.

mavasani

Please remove the unncessary AddCandidateTypesInCompilationUnit method by changing it to a named type symbol analyzer.

heejaechang · 2018-03-24T00:13:37Z

Captured meeting notes from discussion here

sharwell · 2018-03-26T11:20:22Z

Please remove the unncessary AddCandidateTypesInCompilationUnit method by changing it to a named type symbol analyzer.

The current implementation avoids analyzing the contents of nested types multiple times. Switching to a symbol analyzer makes it difficult to preserve this functionality without losing the simplicity benefit of switching to a symbol analyzer, and increases the discrepancy between this and the Use Auto Property analyzer.

mavasani · 2018-03-26T12:58:59Z

The current implementation avoids analyzing the contents of nested types multiple times. Switching to a symbol analyzer makes it difficult to preserve this functionality

I feel this is an extremely rare scenario, especially given our claim here that partial types are not important enough and we are going to skip past large number of types. Personally, I would rather not complicate this already over-complicated implementation to potentially gain perf benefit for corner cases. However, given that we plan to fix the compilation end action bug soon, and delete this implementation, I am okay with this approach.

increases the discrepancy between this and the Use Auto Property analyzer.

I don't think that should be relevant.

sharwell · 2018-03-26T13:07:36Z

I don't think that should be relevant.

It typically wouldn't be. It only became relevant as a secondary benefit when the primary change didn't yield a clear net advantage to either approach.

jinujoseph · 2018-03-27T18:03:35Z

Approval pending for 15.7 Preview 4 (new feature )

jinujoseph · 2018-03-27T22:07:22Z

Approved to merge the feature in 15.7 Preview 4 via Link ( make sure to address review feedback and conflict ) cc @Pilchie for info

Pilchie · 2018-03-28T02:13:51Z

Hooray! 🎆

Hosch250 · 2018-04-02T14:44:46Z

Great job, everyone.

dnfclas added the cla-already-signed label Apr 28, 2017

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 28, 2017

Pilchie added the Area-IDE label Apr 28, 2017

CyrusNajmabadi reviewed Apr 28, 2017

View reviewed changes

sharwell requested changes Apr 28, 2017

View reviewed changes

sharwell reviewed Apr 28, 2017

View reviewed changes

sharwell added 3 commits March 23, 2018 10:42

Use a semantic model analyzer and avoid partial types to improve perf…

cd50cab

…ormance

Fix support for As New clause in VB

d2dc228

Fix incorrect use of async void in code fix

e81622b

sharwell approved these changes Mar 23, 2018

View reviewed changes

mavasani reviewed Mar 23, 2018

View reviewed changes

mavasani suggested changes Mar 23, 2018

View reviewed changes

jinujoseph mentioned this pull request Mar 24, 2018

CompilationEndAction not being called. #11750

Closed

mavasani approved these changes Mar 26, 2018

View reviewed changes

jinujoseph added the Needs Shiproom Approval label Mar 27, 2018

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Mar 27, 2018

Merge remote-tracking branch 'dotnet/dev15.7.x' into Hosch250/master

6c6605b

sharwell merged commit 09bafa5 into dotnet:dev15.7.x Mar 28, 2018

Therzok mentioned this pull request Apr 2, 2018

Slow RefactoringEssentials analyzers mono/monodevelop#4334

Closed

Add Make Field Readonly analyzer/code fix #19067

Add Make Field Readonly analyzer/code fix #19067

Conversation

Hosch250 commented Apr 28, 2017 • edited by jinujoseph Loading

Specs

JohanLarsson commented Apr 28, 2017 • edited Loading

JohanLarsson commented Apr 28, 2017

Pilchie commented Apr 28, 2017

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

sharwell Apr 28, 2017 • 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

sharwell Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

sharwell Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 28, 2017

Hosch250 commented Apr 28, 2017

Hosch250 commented Apr 28, 2017

CyrusNajmabadi commented Apr 28, 2017

Hosch250 commented Apr 29, 2017

sharwell commented Apr 29, 2017

Hosch250 commented Apr 29, 2017

Hosch250 commented Apr 29, 2017

sharwell commented Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 23, 2018

sharwell commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018

sharwell commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018 • edited Loading

sharwell commented Mar 23, 2018 • edited Loading

mavasani left a comment

Choose a reason for hiding this comment

heejaechang commented Mar 24, 2018 • edited by jinujoseph Loading

sharwell commented Mar 26, 2018

mavasani commented Mar 26, 2018

sharwell commented Mar 26, 2018

jinujoseph commented Mar 27, 2018

Hosch250 commented Apr 28, 2017 •

edited by jinujoseph

Loading

JohanLarsson commented Apr 28, 2017 •

edited

Loading

sharwell Apr 28, 2017 •

edited

Loading

sharwell Apr 28, 2017 •

edited

Loading

sharwell Apr 28, 2017 •

edited

Loading

sharwell commented Apr 29, 2017 •

edited

Loading

CyrusNajmabadi commented Mar 23, 2018 •

edited

Loading

sharwell commented Mar 23, 2018 •

edited

Loading

heejaechang commented Mar 24, 2018 •

edited by jinujoseph

Loading

jinujoseph commented Mar 27, 2018 •

edited

Loading