Target framework sets #1668

thomaslevesque · 2019-10-18T17:32:29Z

To be merged after #1667

thomaslevesque · 2019-10-20T19:48:54Z

This PR is on top of #1667 so that it builds successfully. The first 2 commits won't be part of it after #1667 is merged.

blairconrad

I'm having some difficulties with this. In particular, the netcore3.0 set doesn't work for me (I'm still on a preview at work, though). First, I hacked the build program to make it easier for me to work with:

diff --git a/tools/FakeItEasy.Build/Program.cs b/tools/FakeItEasy.Build/Program.cs
index dd9e571..b43bb81 100644
--- a/tools/FakeItEasy.Build/Program.cs
+++ b/tools/FakeItEasy.Build/Program.cs
@@ -56,11 +56,20 @@ namespace FakeItEasy.Build
 
         public static void Main(string[] args)
         {
+            var targetFrameworkSet = "full";
+            foreach (var arg in args)
+            {
+                if (arg.StartsWith("TargetFrameworkSet="))
+                {
+                    targetFrameworkSet = arg.Split('=')[1];
+                }
+            }
+
             Target("default", DependsOn("unit", "integ", "spec", "approve", "pack"));
 
             Target(
                 "build",
-                () => Run("dotnet", "build FakeItEasy.sln -c Release /maxcpucount /nr:false /verbosity:minimal /nologo /bl:artifacts/logs/build.binlog"));
+                () => Run("dotnet", $"build FakeItEasy.sln -c Release /maxcpucount /nr:false /verbosity:minimal /nologo /bl:artifacts/logs/build.binlog /p:TargetFrameworkSet={targetFrameworkSet}"));
 
             foreach (var testSuite in TestSuites)
             {
@@ -68,14 +77,14 @@ namespace FakeItEasy.Build
                     testSuite.Key,
                     DependsOn("build"),
                     forEach: testSuite.Value,
-                    action: testDirectory => Run("dotnet", "test --configuration Release --no-build -- RunConfiguration.NoAutoReporters=true", testDirectory));
+                    action: testDirectory => Run("dotnet", $"test --configuration Release --no-build /p:TargetFrameworkSet={targetFrameworkSet} -- RunConfiguration.NoAutoReporters=true", testDirectory));
             }
 
             Target(
                 "pack",
                 DependsOn("build", "pdbgit"),
                 forEach: ProjectsToPack,
-                action: project => Run("dotnet", $"pack {project.Path} --configuration Release --no-build --output {Path.GetFullPath("artifacts/output")}"));
+                action: project => Run("dotnet", $"pack {project.Path} --configuration Release --no-build --output {Path.GetFullPath("artifacts/output")} /p:TargetFrameworkSet={targetFrameworkSet}"));
 
             Target(
                 "pdbgit",
@@ -83,7 +92,7 @@ namespace FakeItEasy.Build
                 forEach: Pdbs.Where(pdb => File.Exists(pdb.Path)),
                 action: pdb => Run(ToolPaths.PdbGit, $"-u https://github.com/FakeItEasy/FakeItEasy -s {pdb.Path}"));
 
-            RunTargetsAndExit(args, messageOnly: ex => ex is NonZeroExitCodeException);
+            RunTargetsAndExit(args.Where(arg => !arg.Contains('=')), messageOnly: ex => ex is NonZeroExitCodeException);
         }
 
         private class Pdb

Then I ran .\build.cmd TargetFrameworkSet=netcore3.0 and got:

D:\Sandbox\FakeItEasy\tests\FakeItEasy.Tests.TestHelpers\FakeItEasy.Tests.TestHelpers.csproj :
error NU1201: Project FakeItEasy is not compatible with netstandard2.0 (.NETStandard,Version=v2.0).
Project FakeItEasy supports: netstandard2.1 (.NETStandard,Version=v2.1)
[D:\Sandbox\FakeItEasy\FakeItEasy.sln]

Probably because we didn't build FIE 2.0 in this run.
Indeed, changing TestHelpersTargetFrameworks to netstandard2.1 for this TargetFrameworkSet resolved the problem…

Or maybe I'm doing things wrong, since the "full" build works fine.

blairconrad · 2019-10-21T11:04:38Z

tests/FakeItEasy.Tests.TestHelpers/FakeItEasy.Tests.TestHelpers.csproj

+  </PropertyGroup>
+
+  <PropertyGroup Condition="'$(TargetFramework)' == 'netstandard1.6'">
+    <DefineConstants>$(DefineConstants);FEATURE_NETCORE_REFLECTION;USE_RUNTIMELOADER;FEATURE_EXCEPTION_DISPATCH_INFO;FEATURE_ARRAY_EMPTY;FEATURE_PARAMETERINFO_CUSTOMATTRIBUTES_PROPERTY</DefineConstants>


I am surprised some of these constants (e.g. FEATURE_ARRAY_EMPTY) don't need to be set for netstandard2.0. Or at least can't be set…

Oh. I must have copied this from FakeItEasy.csproj... We don't use FEATURE_ARRAY_EMPTY in the tests anyway. I'll fix this and leave only the necessary ones.

I think we both forgot about this comment as well.

Oops! Sorry

blairconrad · 2019-10-21T11:06:28Z

tests/FakeItEasy.Tests.TestHelpers/ObjectAssertionsExtensions.cs

@@ -17,7 +17,10 @@ public static class ObjectAssertionsExtensions
        /// <param name="assertion">A FluentAssertions assertion that has been initiated on a subject.</param>
        public static void BeAFake(this ReferenceTypeAssertions<object, ObjectAssertions> assertion)
        {
-            Guard.AgainstNull(assertion, nameof(assertion));
+            if (assertion == null)


rather than make FakeItEasy's internals visible to this project, I assume?

Yes. At this point. If you prefer, I can add a InternalsVisibleTo attribute to FIE

I don't mind either way. What you have is fine. Thanks for asking.

blairconrad · 2019-10-21T13:19:05Z

Directory.Build.props

+
+  <!-- .NET Framework only -->
+  <PropertyGroup Condition=" '$(TargetFrameworkSet)' == 'netfx' ">
+    <FakeItEasyTargetFrameworks>net45</FakeItEasyTargetFrameworks>


and net40 as well?

Since the goal was to make the build faster, I thought it would make sense to include as few TFMs as possible. Is there a scenario where we would want to build all netfx targets, and nothing else?
But the target sets I defined here are mostly examples anyway. The idea is that we can add new ones as needed.

Ah. I was taking "netcoreall" for comparison, without paying attention to the "all" on the end of that.
I don't mind excluding net40 from the set, but maybe in this case, rename to net45, since we're closer to a "netcore2.1" situation than a "netcoreall" situation?

Good point. I don't think a netfxall is very useful. I intended netcoreall to be the default for non-Windows build (could be set automatically based on the OS)

blairconrad · 2019-10-21T13:31:12Z

Directory.Build.props

+    <ApprovalTestsTargetFrameworks>net461</ApprovalTestsTargetFrameworks>
+  </PropertyGroup>
+
+  <!-- .NET Core 3 / .NET Standard 2.0 -->


.NET Standard 2.1?

Yes! Forgot to update after I rebased

blairconrad · 2019-10-21T13:31:35Z

Directory.Build.props

+    <IntegrationTestsTargetFrameworks>netcoreapp3.0</IntegrationTestsTargetFrameworks>
+    <UnitTestsTargetFrameworks>netcoreapp3.0</UnitTestsTargetFrameworks>
+    <AnalyzerTestsTargetFrameworks>netcoreapp3.0</AnalyzerTestsTargetFrameworks>
+    <TestHelpersTargetFrameworks>netstandard2.0</TestHelpersTargetFrameworks>


I'm not sure about this. See my summary.

thomaslevesque · 2019-10-21T13:49:33Z

In particular, the netcore3.0 set doesn't work for me

Oops. Seems broken for me too. Must have broke when I rebased. I'll look into it.

thomaslevesque · 2019-10-21T13:54:33Z

In general, what do you think about this approach?
I like your idea to add a parameter to the build script.
We could also enable a user-defined props file that would be included in Directory.Build.props if it exists, and ignored by Git. e.g.

    <Import Project="$(MSBuildThisFileDirectory)/FakeItEasy.user.props" Condition="Exists('$(MSBuildThisFileDirectory)/FakeItEasy.user.props')" />

This way, there would be no need to temporarily edit Directory.Build.props

thomaslevesque · 2019-10-21T14:11:48Z

As a possible improvement, I could move the "target framework set definitions" to separate files, which we import based on the value of TargetFrameworkSet property:

<Import Project="targets/$(TargetFrameworkSet).props" />

This would be mostly to make the Directory.Build.props file look cleaner

Also, I'm not sure about the name of the concept. "Target framework set" is what I came up with, but let me know if you have a better idea. "Build profile", maybe?

blairconrad · 2019-10-21T14:29:12Z

I think the approach is good. I'm not sure I'll use the feature that often, but who knows? I use -s more than I expected.

I liked adding a parameter to the build script as well. The problem is that it's not discoverable as is, and as you and M. Ralph noted, adding parameters that aren't handled by Bullseye can be problematic. I toyed with the idea of adding targets to set the targetframeworkset (e.g. build netcoreall default, but the downside is that as soon as you include the target, you lose the defaultiness of the default target. Maybe that's an acceptable downside? If just running tests it isn't apparent: build netcoreall specs. And honestly, most of the time that I'll be trying to speed up the build, I will for sure be cutting out the approval and packing targets!

We could also enable a user-defined props file that would be included in Directory.Build.props if it exists, and ignored by Git.

Yes, please. Better than temporarily editing anything. Until we edit that file and forget to unedit! 😆

could move the "target framework set definitions" to separate files

I'm ambivalent. Cleaner main file, but more files to look at. Do whichever you think is best.

I'm not sure about the name of the concept. "Target framework set" is what I came up with, but let me know if you have a better idea. "Build profile", maybe?

I found "target framework set" to be comprehensible, so I was content. But since you put "build profile" in front of my face, I find I like it more…

thomaslevesque · 2019-10-21T14:48:53Z

I'm not sure I'll use the feature that often, but who knows?

I think I would. In fact, I think I will almost always set the target framework set to netcore3.0, to have a fast feedback loop while coding, and only switch back to full before committing.

I liked adding a parameter to the build script as well. The problem is that it's not discoverable as is, and as you and M. Ralph noted, adding parameters that aren't handled by Bullseye can be problematic.

Look here for a workaround. Not ideal, but it gets the job done.

I toyed with the idea of adding targets to set the targetframeworkset (e.g. build netcoreall default, but the downside is that as soon as you include the target, you lose the defaultiness of the default target. Maybe that's an acceptable downside? If just running tests it isn't apparent: build netcoreall specs.

It's a good option too. But I think I like something like build.cmd --targets=netcoreall better than build.cmd netcoreall default

Yes, please. Better than temporarily editing anything. Until we edit that file and forget to unedit! 😆

Will do 👍

I'm ambivalent. Cleaner main file, but more files to look at. Do whichever you think is best.

I'll try it and we'll see how it goes.

I found "target framework set" to be comprehensible, so I was content. But since you put "build profile" in front of my face, I find I like it more…

Build profile it is, then!

thomaslevesque · 2019-10-21T15:32:22Z

I pushed new commits for the changes we discussed, let me know what you think.

thomaslevesque · 2019-10-21T15:32:52Z

(I didn't implement the changes in the build script yet)

blairconrad

Looking good, @thomaslevesque. One minor comment, which I don't care that much if you change anything for. I'm content for you to add docs and squash!

blairconrad · 2019-10-21T16:04:52Z

profiles/netcoreall.props

+  <PropertyGroup>
+    <FakeItEasyTargetFrameworks>netstandard1.6;netstandard2.0;netstandard2.1</FakeItEasyTargetFrameworks>
+    <ValueTaskExtensionsTargetFrameworks>netstandard1.6;netstandard2.0;netstandard2.1</ValueTaskExtensionsTargetFrameworks>
+    <ExampleTargetFrameworks>netcoreapp2.1</ExampleTargetFrameworks>


I expected 2.1 and 3.0.
If the thinking was that they're just examples, so we probably don't need to build both, maybe default to the newer framework? I can live with it as is, though.

(Actually, this brings up a similar question for the full profile, and which frameworks we should target for the examples and the approval tests - if we're just going to target one in each build profile, should it be the latest/most portable? Or maybe it's all fine.)

If the thinking was that they're just examples, so we probably don't need to build both

Yes, that's what I had in mind

maybe default to the newer framework?

👍

(Actually, this brings up a similar question for the full profile, and which frameworks we should target for the examples and the approval tests - if we're just going to target one in each build profile, should it be the latest/most portable? Or maybe it's all fine.)

The latest for each profile is a good choice, I think.

thomaslevesque · 2019-10-21T20:01:07Z

I changed the TFM for the example projects, squashed fixup commits, and updated the docs

blairconrad

Sorry, @thomaslevesque. One question and one minor grammatical nit.
Otherwise, great. I enjoyed the docs. Very explainy!

blairconrad · 2019-10-22T01:09:37Z

profiles/netcoreall.props

+    <IntegrationTestsTargetFrameworks>netcoreapp1.0;netcoreapp2.1;netcoreapp3.0</IntegrationTestsTargetFrameworks>
+    <UnitTestsTargetFrameworks>netcoreapp1.0;netcoreapp2.1;netcoreapp3.0</UnitTestsTargetFrameworks>
+    <AnalyzerTestsTargetFrameworks>netcoreapp2.1;netcoreapp3.0</AnalyzerTestsTargetFrameworks>
+    <TestHelpersTargetFrameworks>netstandard1.6;netstandard2.0;netstandard2.1</TestHelpersTargetFrameworks>


Why a netstandard2.1 here but not in full?

A mistake. TestHelpers doesn't need a netstandard2.1 target

And it also shouldn't be netstandard2.1 for the netcore3.0 profile

blairconrad · 2019-10-22T01:13:07Z

how_to_build.md

+FakeItEasy targets multiple versions of .NET (.NET Framework 4.0 and 4.5, .NET
+Standard 1.6, 2.0 and 2.1), and the tests also run on multiple frameworks (.NET
+Framework 4.6.1 and several versions of .NET Core). A consequence is that a full
+build can take a significant time. When working on the code, you might want a


"take a significant time" hits my ear funny. I'd've written "take a significant amount of time". Or perhaps "take significant time".

Ah, thanks. Not being an native English speaker, it can sometimes be hard to tell what will sound funny to a native speaker...

thomaslevesque · 2019-10-22T08:50:38Z

Fixed and squashed

blairconrad

It's the review that never ends. Sorry.

blairconrad · 2019-10-22T10:22:35Z

profiles/netcore3.0.props

+    <IntegrationTestsTargetFrameworks>netcoreapp3.0</IntegrationTestsTargetFrameworks>
+    <UnitTestsTargetFrameworks>netcoreapp3.0</UnitTestsTargetFrameworks>
+    <AnalyzerTestsTargetFrameworks>netcoreapp3.0</AnalyzerTestsTargetFrameworks>
+    <TestHelpersTargetFrameworks>netstandard2.0</TestHelpersTargetFrameworks>


When I use the FakeItEasy.user.props file to set the build profile to netcore3.0 (which was fun), and run build integ, I again get

D:\Sandbox\FakeItEasy\tests\FakeItEasy.Tests.TestHelpers\FakeItEasy.Tests.TestHelpers.csproj : error NU1201: Project FakeItEasy is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Project FakeItEasy supports: netstandard2.1 (.NETStandard,Version=v2.1) [D:\Sandbox\FakeItEasy\FakeItEasy.sln]

Argh... OK, I'll really review and test those TFMs before I push a new fix.

blairconrad · 2019-10-22T10:25:01Z

tests/FakeItEasy.Tests.TestHelpers/FakeItEasy.Tests.TestHelpers.csproj

+  </PropertyGroup>
+
+  <PropertyGroup Condition="'$(TargetFramework)' == 'netstandard1.6'">
+    <DefineConstants>$(DefineConstants);FEATURE_NETCORE_REFLECTION;USE_RUNTIMELOADER;FEATURE_EXCEPTION_DISPATCH_INFO;FEATURE_ARRAY_EMPTY;FEATURE_PARAMETERINFO_CUSTOMATTRIBUTES_PROPERTY</DefineConstants>


I think we both forgot about this comment as well.

And upgrade ApprovalTests

thomaslevesque · 2019-10-22T12:10:40Z

It's the review that never ends. Sorry.

My fault... It should be fixed now
Thanks for the careful review!

blairconrad · 2019-10-22T16:59:29Z

Hey. Packing fails in a clean sandbox with a non-full build. e.g. when I build netcore3.0. Perhaps we should take a page out of the main package's book and use something like

<None Include="$(OutputPath)/**/$(AssemblyName).pdb" Pack="true" PackagePath="lib" Visible="false" />

in FakeItEasy.Extensions.ValueTask.csproj

thomaslevesque · 2019-10-22T17:54:01Z

Packing fails in a clean sandbox with a non-full build. e.g. when I build netcore3.0.

Oh :(
I had tried all profiles, but only with the build target.

Perhaps we should take a page out of the main package's book and use something like

Yes, I think I'll do that. Although I intended to drop that at some point and use AllowedOutputExtensionsInPackageBuildOutputFolder instead, as mentioned here.

thomaslevesque · 2019-10-22T18:10:23Z

Perhaps we should take a page out of the main package's book and use something like

In fact, it doesn't work, because it also takes netstandard2.1, which should be excluded...

blairconrad · 2019-10-22T18:47:36Z

<None Include="$(OutputPath)/net45/$(AssemblyName).pdb" Pack="true" PackagePath="lib/net45" Visible="false" Condition="Exists('$(OutputPath)/net45/$(AssemblyName).pdb')" />
<None Include="$(OutputPath)/netstandard1.6/$(AssemblyName).pdb" Pack="true" PackagePath="lib/netstandard1.6" Visible="false" Condition="Exists('$(OutputPath)/netstandard1.6/$(AssemblyName).pdb')" />
<None Include="$(OutputPath)/netstandard2.0/$(AssemblyName).pdb" Pack="true" PackagePath="lib/netstandard2.0" Visible="false" Condition="Exists('$(OutputPath)/netstandard2.0/$(AssemblyName).pdb')" />

?

thomaslevesque · 2019-10-22T19:27:43Z

It's an option. But if the PDBs are there from a previous build, it would include them even when they're not in the selected TFMs. I have another option in mind, let me try...

thomaslevesque · 2019-10-22T21:18:08Z

I have another option in mind, let me try...

Well, my thing didn't work. I found another way, which isn't great, but the end result is slightly cleaner than before

But if the PDBs are there from a previous build, it would include them even when they're not in the selected TFMs

Actually, it doesn't really matter, because we will only ever publish packages produced by the full build anyway.

thomaslevesque · 2019-10-22T21:19:37Z

src/FakeItEasy.Extensions.ValueTask/FakeItEasy.Extensions.ValueTask.csproj

    <SignAssembly>true</SignAssembly>
+    <IncludeBuildOutput>false</IncludeBuildOutput>


This causes the Pack target to not include .dll and .xml files from the build output. I include them manually, which gives me control over what goes into the package.

thomaslevesque · 2019-10-22T21:20:20Z

src/FakeItEasy.Extensions.ValueTask/FakeItEasy.Extensions.ValueTask.csproj

    <SignAssembly>true</SignAssembly>
+    <IncludeBuildOutput>false</IncludeBuildOutput>
+    <BeforePack>AddNetStd21Placeholder;$(BeforePackOn)</BeforePack>


Cleaner than depending on _WalkEachTargetPerFramework, which is a private target

thomaslevesque · 2019-10-22T21:22:17Z

src/FakeItEasy.Extensions.ValueTask/FakeItEasy.Extensions.ValueTask.csproj

+    <None Include="$(OutputPath)/**/$(AssemblyName).dll" Pack="true" PackagePath="lib" Visible="false" />
+    <None Include="$(OutputPath)/**/$(AssemblyName).pdb" Pack="true" PackagePath="lib" Visible="false" />
+    <None Include="$(OutputPath)/**/$(AssemblyName).xml" Pack="true" PackagePath="lib" Visible="false" />
+    <None Remove="$(OutputPath)/netstandard2.1/*" />


I include everything, then exclude what I don't want

blairconrad · 2019-10-22T22:19:31Z

Nice job, @thomaslevesque! Thanks for your work.

thomaslevesque · 2019-10-23T07:40:57Z

Thanks for the careful review!

thomaslevesque force-pushed the target-framework-sets branch 2 times, most recently from 2a704a3 to fe3fcb0 Compare October 20, 2019 19:45

thomaslevesque force-pushed the target-framework-sets branch from fe3fcb0 to 4b0048a Compare October 21, 2019 10:33

thomaslevesque changed the title ~~[DO NOT MERGE] Target framework sets~~ Target framework sets Oct 21, 2019

blairconrad reviewed Oct 21, 2019

View reviewed changes

thomaslevesque force-pushed the target-framework-sets branch from 4b0048a to d7cc4bd Compare October 21, 2019 14:07

blairconrad requested changes Oct 21, 2019

View reviewed changes

thomaslevesque force-pushed the target-framework-sets branch from f2aa766 to 9bafe0c Compare October 21, 2019 19:59

blairconrad requested changes Oct 22, 2019

View reviewed changes

thomaslevesque force-pushed the target-framework-sets branch from 9bafe0c to 30a4801 Compare October 22, 2019 08:50

blairconrad requested changes Oct 22, 2019

View reviewed changes

thomaslevesque added 2 commits October 22, 2019 13:58

Extract test helpers to separate project

02cb42c

Skip approval and PdbGit for TFMs not built

22b25d8

And upgrade ApprovalTests

thomaslevesque force-pushed the target-framework-sets branch from 30a4801 to 76c1139 Compare October 22, 2019 11:58

thomaslevesque added 4 commits October 22, 2019 23:17

Add target framework sets

2a5e445

Add support for user-defined props file

3fcff7e

Move build profiles to separate files

88d7700

Update documentation

2d1efaf

thomaslevesque force-pushed the target-framework-sets branch from 76c1139 to 2d1efaf Compare October 22, 2019 21:17

thomaslevesque commented Oct 22, 2019

View reviewed changes

blairconrad approved these changes Oct 22, 2019

View reviewed changes

blairconrad merged commit c2c47b2 into FakeItEasy:master Oct 22, 2019

thomaslevesque deleted the target-framework-sets branch October 23, 2019 07:40

		<SignAssembly>true</SignAssembly>
		<IncludeBuildOutput>false</IncludeBuildOutput>

Target framework sets #1668

Target framework sets #1668

Conversation

thomaslevesque commented Oct 18, 2019

thomaslevesque commented Oct 20, 2019

blairconrad left a 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

thomaslevesque commented Oct 21, 2019

thomaslevesque commented Oct 21, 2019

thomaslevesque commented Oct 21, 2019

blairconrad commented Oct 21, 2019

thomaslevesque commented Oct 21, 2019

thomaslevesque commented Oct 21, 2019

thomaslevesque commented Oct 21, 2019

blairconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaslevesque Oct 21, 2019 • edited

Choose a reason for hiding this comment

thomaslevesque commented Oct 21, 2019

blairconrad left a 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, 2019

blairconrad left a 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, 2019

blairconrad commented Oct 22, 2019 • edited

thomaslevesque commented Oct 22, 2019

thomaslevesque commented Oct 22, 2019

blairconrad commented Oct 22, 2019 • edited

thomaslevesque commented Oct 22, 2019

thomaslevesque commented Oct 22, 2019

thomaslevesque Oct 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blairconrad commented Oct 22, 2019

thomaslevesque commented Oct 23, 2019

thomaslevesque Oct 21, 2019 •

edited

blairconrad commented Oct 22, 2019 •

edited

blairconrad commented Oct 22, 2019 •

edited

thomaslevesque Oct 22, 2019 •

edited