Create nuget package #56

ermshiperete · 2019-04-08T14:08:34Z

This change will make it possible to create nuget packages for the xliff branch (l10nsharp.xliff).

This change is

marksvc

lgtm. I didn't follow all of the L10NSharp.proj logic.
I rarely/never build l10n sharp, so it would make sense for someone with more stake to give an architectural approval / opinion.

Reviewed 28 of 28 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @ermshiperete, @hatton, and @StephenMcConnel)

build/NuGet.targets, line 26 at r1 (raw file):

		<NuGetToolsPath>$(MSBuildThisFileDirectory)</NuGetToolsPath>
		<PackagesConfig Condition="'$(PackagesConfig)'=='' AND '$(OS)'=='Windows_NT'">$(ProjectDir)packages.config</PackagesConfig>
		<PackagesConfig Condition="'$(PackagesConfig)'=='' AND '$(OS)'!='Windows_NT'">$(ProjectDir)/packages.config</PackagesConfig>

That's unfortunate.

build/NuGet.targets, line 64 at r1 (raw file):

Period at the end? Presumably SolutionDir ends in / or \ and /. or \. is important for specifying the directory?
(Or it's a mistake?)

build/NuGet.targets, line 67 at r1 (raw file):

	</Target>

	<UsingTask TaskName="DownloadNuGet" TaskFactory="CodeTaskFactory"

I'm guessing that this is Windows-only because of Mono SSL complications in the past. We may be able to start using this now with Mono 5. Something we can try some time.

src/L10NSharp/UI/Utils.cs, line 11 at r1 (raw file):

	internal static class Utils
	{
		private static bool?  _isMono;

This might be able to be readonly. I don't know if that would give any micro performance boost, but maybe.
Good use of bool?.

src/L10NSharpTests/XLiffUtilsTests.cs, line 23 at r1 (raw file):

			folder = Path.GetDirectoryName(folder);
			folder = Path.GetDirectoryName(folder);
			folder = Path.GetDirectoryName(folder);

Okay. But: I wonder if something like this would work here:

var folder = Path.GetDirectoryName(asmFile); // will be something like <repodir>/output/Debug
folder = Path.Combine(folder,"../../..");
_testFolder = ...

or maybe better

var asmDir = Path.GetDirectoryName(asmFile); // will be something like <repodir>/output/Debug
_testFolder = Path.Combine(asmDir, "../../..", "src", "L10NSharpTests", "TestXliff");

(Or depending on how Path.Combine works, maybe _testFolder = Path.Combine(asmDir, "..","..","..", "src", "L10NSharpTests", "TestXliff");

I won't impose on you to make this nicer as part of your current patches :)

ermshiperete

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @hatton, @marksvc, and @StephenMcConnel)

build/NuGet.targets, line 26 at r1 (raw file):

Previously, marksvc wrote…

That's unfortunate.

I'm not entirely sure that it's needed. That's just copied from the master branch.

build/NuGet.targets, line 64 at r1 (raw file):

Previously, marksvc wrote…

.
Period at the end? Presumably SolutionDir ends in / or \ and /. or \. is important for specifying the directory?
(Or it's a mistake?)

That's a hack for the cases where SolutionDir isn't set. But again, that was just copied from master.

build/NuGet.targets, line 67 at r1 (raw file):

Previously, marksvc wrote…

I'm guessing that this is Windows-only because of Mono SSL complications in the past. We may be able to start using this now with Mono 5. Something we can try some time.

yes, that's Windows-only.

src/L10NSharp/UI/Utils.cs, line 11 at r1 (raw file):

Previously, marksvc wrote…

This might be able to be readonly. I don't know if that would give any micro performance boost, but maybe.
Good use of bool?.

To make it readonly we'd have to set it in the static c'tor.

src/L10NSharpTests/XLiffUtilsTests.cs, line 23 at r1 (raw file):

Previously, marksvc wrote…

Okay. But: I wonder if something like this would work here:
var folder = Path.GetDirectoryName(asmFile); // will be something like <repodir>/output/Debug
folder = Path.Combine(folder,"../../..");
_testFolder = ...
or maybe better
var asmDir = Path.GetDirectoryName(asmFile); // will be something like <repodir>/output/Debug
_testFolder = Path.Combine(asmDir, "../../..", "src", "L10NSharpTests", "TestXliff");
(Or depending on how Path.Combine works, maybe _testFolder = Path.Combine(asmDir, "..","..","..", "src", "L10NSharpTests", "TestXliff");

I won't impose on you to make this nicer as part of your current patches :)

Yes, that could definitely use some refactoring. Just went with the easiest solution (which is extend what was already there) 😄

StephenMcConnel

There's a bit of awkward English I'd like to see improved, one trivial (and optional) formatting change, and one question that affects several test files: where exactly does the l10nsharptests.dll end up? I couldn't see where its target destination changed, but a number of files assume it get buried one level deeper in the directory tree. But I'm willing to believe I missed something in one of the csproj files.
Otherwise it looks good as far as I can tell.

Reviewed 28 of 28 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @andrew-polk, @ermshiperete, @hatton, and @marksvc)

README.md, line 39 at r1 (raw file):

The L10NSharp project uses SDK style .csproj files which doesn't allow to use the Designer in Visual Studio 2017.
There is a `src\L10NSharp\L10NSharp-Designer.csproj` file which uses the old .csproj style and thus allows to
edit the files in the designer.

I would reword the English here in a couple of places. Something like the following:

The L10NSharp project uses SDK style .csproj files which don't allow the Designer to be used in Visual Studio 2017.
There is a src\L10NSharp\L10NSharp-Designer.csproj file which uses the old .csproj style and thus allows the programmer to edit the files with the Designer in Visual Studio 2017.

build/NuGet.targets, line 29 at r1 (raw file):

		<!-- NuGet command -->
		<NuGetExePath Condition=" '$(NuGetExePath)' == '' ">$(NuGetToolsPath)/nuget.exe</NuGetExePath>

I trust you've verified that the nuget.exe obtained here is all lowercase and not CamelCased. :-)

src/ExtractXliff/Program.cs, line 13 at r1 (raw file):

using System.Resources;
using System.Globalization;
using System.Collections;

I assume these were all unused since the program has no GUI or mulit-threading. (Which I should know, being the original author.)

src/L10NSharp/UI/Utils.cs, line 66 at r1 (raw file):

				}
				else
					SendMessage(ctrl.Handle, WM_SETREDRAW, (turnOn ? 1 : 0), 0);

I would prefer matching braces P {} since the other branch of the if/else used braces. But that may just be a personal preference. But the actual logic looks okay.

src/L10NSharpTests/ILocalizableComponentTests.cs, line 28 at r1 (raw file):

		public void TestSetup()
		{
			var installedXliffDir = "../../../src/L10NSharpTests/TestXliff";

Are you sure this extra ../ is needed? I suppose it depends on where the test dll is placed, and that may have changed with the revised build process.

Mono 5 uses the Roslyn compiler (which is the same as on Windows) which doesn't define __MonoCS__. This change modifies the code to check at runtime whether we're running on Mono.

This change converts the .csproj file to the sdk based format which is a lot smaller and allows to define various properties in the csproj file. The downside is that I'm not sure if the forms can still be opened in the designer from the project view, and the designer files are disconnected from their other files.

Also add SIL.ReleaseTasks package to automatically add release notes to nuget package. +semver: minor

msbuild /t:Test build/L10NSharp.proj

This project file can be used to edit files in the designer in Visual Studio 2017.

ermshiperete

All assemblies end up in output/$(configuration)/net461 with the new sdk-style .csproj files - even though net461 isn't mentioned in the definition of OutputPath.

Reviewable status: 26 of 28 files reviewed, 8 unresolved discussions (waiting on @andrew-polk, @hatton, @marksvc, and @StephenMcConnel)

README.md, line 39 at r1 (raw file):