Multi targeting #276

andrewheumann · 2020-04-02T01:44:11Z

BACKGROUND:

In order to support the Grasshopper plug-in and other .NET Framework applications, Elements is expanded to target multiple frameworks

DESCRIPTION:

Target .Net Framework 4.7.2 and .Net Core 2.1 in addition to netstandard2.0.
Establish conditional compile constants for any changes necessary to support 4.7.2
Refactor and expand Elements.Generate.TypeGenerator to support the dynamic type compilation needs of the Grasshopper plug-in
Minor code changes to fix errors that threw in Framework builds (e.g. Guid id = Guid.Empty;)

TESTING:

Have tested with the Grasshopper plug-in and run all Unit Tests

FUTURE WORK:

Additional configuration may be necessary to correctly support a multi-targeted NuGet package (https://docs.microsoft.com/en-us/nuget/create-packages/supporting-multiple-target-frameworks)

This change is

…ments into multiTargeting

wynged

Do we have any system setup to test the separate targets? How will we know if we use a feature in one framework setting that doesn't work as expected? Or are compilation errors the only real concern.

Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @andrewheumann and @ikeough)

src/Elements/Elements.csproj, line 4 at r1 (raw file):

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;net472;netcoreapp2.1</TargetFrameworks>

Do we need all three of these? why netstandard2.0 AND netcoreapp2.1?

andrewheumann

I'm glad you asked about this @wynged — In theory it's possible to set up multi-targeting tests, although I'm having a little trouble doing this. Currently all tests pass in .netstandard and it compiles in both, but so far I've been unable to get tests running in .net framework.

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @ikeough)

src/Elements/Elements.csproj, line 4 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

Do we need all three of these? why netstandard2.0 AND netcoreapp2.1?

when @ikeough and I set this up we put in both, I forget why. @ikeough is there any reason to maintain netcoreapp2.1 as well?

ikeough

Reviewable status: 2 of 9 files reviewed, 8 unresolved discussions (waiting on @andrewheumann, @ikeough, and @wynged)

src/Elements/Elements.csproj, line 4 at r1 (raw file):

Previously, andrewheumann wrote…

when @ikeough and I set this up we put in both, I forget why. @ikeough is there any reason to maintain netcoreapp2.1 as well?

I don't think we need netcoreapp2.1. netstandard2.0 should cover it.

src/Elements/Generate/TypeGenerator.cs, line 84 at r2 (raw file):

            var schema = GetSchema(uri);
            var csharp = GenerateCodeForSchema(schema);
            Console.WriteLine($"Writing to {outPath}...");

"Generating type Foo in users/superdave/Foo.g.cs..."

src/Elements/Generate/TypeGenerator.cs, line 104 at r2 (raw file):

                    var schema = GetSchema(uri);
                    var csharp = GenerateCodeForSchema(schema);
                    if (csharp == null) continue;

Brackets

src/Elements/Generate/TypeGenerator.cs, line 152 at r2 (raw file):

        /// </summary>
        /// <param name="uri">The web URL or file path to the schema JSON.</param>
        /// <returns></returns>

Remove empty <returns> if you're not using it.

src/Elements/Generate/TypeGenerator.cs, line 244 at r2 (raw file):

        /// </summary>
        /// <returns></returns>
        public static List<Type> GetLoadedTypeNames()

It seems that this should be List<Element> GetLoadedElementTypes()

src/Elements/Generate/TypeGenerator.cs, line 270 at r2 (raw file):

        /// </summary>
        /// <returns></returns>
        public static string GetAssemblyFolder()

Does this need to be public?

test/Elements.Tests/Grid1dTests.cs, line 37 at r3 (raw file):

            var pattern = new[] { 1.0, 1.5 };
#if NETCORE
            grid[3].DivideByPattern(pattern);

Why is this only allowed on .net core?

test/Elements.Tests/Grid1dTests.cs, line 228 at r3 (raw file):

        }

        [IgnoreOnNetFrameworkFact]

Why are we ignoring these tests?

andrewheumann

Reviewable status: 2 of 9 files reviewed, 8 unresolved discussions (waiting on @ikeough and @wynged)

src/Elements/Elements.csproj, line 4 at r1 (raw file):