Build time ResX code-behind generation #1782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

wjk wants to merge 22 commits into dotnet:master from wjk:build-time-resx-generation

wjk commented Dec 1, 2017 •

edited

Loading

This is my stab at resolving #94. To use it in a project (via dogfood.cmd or similar), do the following:

Add a resx file to your project; delete the code-behind file created by Visual Studio.
Change the <EmbeddedResource> line in the csproj to have the metadata GenerateCodeBehind="true".
Rebuild. A code-behind should be generated in the $(IntermediateOutputDir) and be available to IntelliSense.

Other things to note:

You can now change the doc comment placed into the generated code by setting the DocCommentMode metadata. You can set it to None to disable the doc comment entirely, ResxComment to use the comment set in the VS resx editor, or to Content (the default) to use the content of the string resource, unaltered.
There is still no support for F#, nor for non-string resources.

Author

wjk commented Dec 1, 2017

I have also taken the liberty of using the comment entered in the ResX editor (or in the <comment> tag if the XML is edited by hand) as the doc comment for the generated property. If one is not present, then a default comment is generated using the value of the resource, similar to what the VS generator does today.

I have been wishing that VS would do this for me for many years. Since we are finally rewriting the ResX generator, I figured that this would be a good opportunity to add this functionality.

Contributor

nguerrera commented Dec 19, 2017

@wjk Sorry for the delay in looking at this and thank you for working on it. I won't be able to review in detail until after the holidays, but let me at least answer this now to unblock ad-hoc testing:

I don't know how to force override the SDK bundled with MSBuild/VS). Any pointers on either of these topics will be appreciated.

You can run build\dogfood.cmd for this after you have done a build. This will set up a powershell session with environment variables such that dotnet build, msbuild, and devenv will all use the SDK you just built.

That script was broken recently, so you'll want to merge with latest master first.

nguerrera reviewed

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/GenerateResxCodeBehindFile.cs Outdated

+                              // If the comment was provided by the developer, use that.
+                              // Otherwise, generate a default comment like the old ResX generator does.
+                              if (comment == null)

Contributor

nguerrera Dec 19, 2017

In our own usage, comments are usually notes to translators. I think it would be useful to see both the English text AND the comment in quick info.

nguerrera reviewed

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/GenerateResxCodeBehindFile.cs Outdated

+                              try
+                              {
+                                  if (Language.ToUpperInvariant() == "C#")

Contributor

nguerrera Dec 19, 2017

Language.Equals("C#", StringComparison.OrdinalIgnoreCase)

nguerrera reviewed

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/GenerateResxCodeBehindFile.cs Outdated

+                              foreach (var line in escapedTrimmedValue.Split(new[] { "\r\n", "\r", "\n" }, StringSplitOptions.None))
+                                  builder.AppendLine($"{memberIndent}/// {line}");
+                              string identifier = IsLetterCharacter(name[0]) ? name : "_" + name;

Contributor

nguerrera Dec 19, 2017

This is only one case of an invalid identifier. I will see if we can find the code from the CodeDOM-based generator in order to get parity with it. I see that it knows to make "a b", "a_b", and even "for" into "_for".

nguerrera reviewed

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/GenerateResxCodeBehindFile.cs Outdated

+                          {
+                              string name = node.Attribute("name")?.Value ?? throw new InvalidDataException("Missing resource name");
+                              string value = node.Elements("value").FirstOrDefault()?.Value.Trim() ?? throw new InvalidDataException($"Missing resource value: '{name}'");
+                              string comment = node.Elements("comment").FirstOrDefault()?.Value.Trim();

Contributor

nguerrera Dec 19, 2017

Another interesting case is non-string values...

wjk force-pushed the build-time-resx-generation branch from 04faed1 to 82dbf8c Compare

December 20, 2017 01:34

livarcocc added this to the 2.2.0 milestone

kant2002 reviewed

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/GenerateResxCodeBehindFile.cs Outdated

+                                  }
+                                  else
+                                  {
+                                      throw new BuildErrorException($"Unrecognized language '{Language}'");

kant2002 Jan 9, 2018

Would be any plans to support F# for ResX code-generation?

Author

wjk Jan 9, 2018

@kant2002 Not in this PR, I'm afraid; I don't know a thing about F#. Sorry about that!

Contributor

dasMulli commented Jan 9, 2018

MSBuild has
https://github.com/Microsoft/msbuild/blob/master/src/Tasks/StronglyTypedResourceBuilder.cs
https://github.com/Microsoft/msbuild/blob/a9f64ebd108702c3fc65339c66cb124217854524/src/Tasks/GenerateResource.cs#L2445
which are controlled by a few inputs: https://github.com/Microsoft/msbuild/blob/8ee3c06d0a1b6a0b424fd69d8eb9222dec4543d1/src/Tasks/Microsoft.Common.CurrentVersion.targets#L2997-L3001

Would it make sense to extend msbuild itself for non-FEATURE_CODEDOM with this code?

Contributor

dasMulli commented Jan 9, 2018

And then add defaults for the metadata items for the strongly typed resource attributes in the SDK.

Author

wjk commented Jan 9, 2018

@dasMulli I can certainly try that approach; however, I will need to copy and modify the task code to make it fully work as desired here. If that's OK, then I'd love to try it!

wjk added 11 commits

September 23, 2018 20:36


          Add MSBuild tasks for resx code-behind generation

0c65da9

These are not yet wired up into any *.targets files,
nor are they tested as of this commit.


          Rename and simplify GetResxCodeBehindFilePath

e68e3a7

Also implemented a first stab at disabling code-behind generation
for ResX files for which doing so is undesirable (e.g. WinForms
form resource files).


          Enable configuration of if resx code-behinds should be generated by d…

3327c57

…efault


          Don't auto-generate code-behinds managed by VS

0d5692e

This fix can be removed when <dotnet#1199>
is resolved, and when the Visual Studio code that maintains these
files is disabled when using the SDK-based project system.


          Rename GenerateResxSource task for consistency


          GenerateResxCodeBehindFile now specifies outputs

012795c


          Add and wire up GenerateResxCodeBehind.targets

59a4372


          Addendum to ab38b47

22bb4c3


          Run ResX code-behind generation before the Compile target

538a15b


          Fix incremental builds

2343c01


          Fix string comparison

bdb277f

wjk force-pushed the build-time-resx-generation branch from 280ff49 to bdb277f Compare

September 24, 2018 00:36

wjk added 2 commits

September 23, 2018 20:44


          Fix build

07b15d7


          Update to newer algorithm from Arcade

45ddeb1

Note that this code still has not yet been tested.

Author

wjk commented Sep 24, 2018

Update on the state of this PR:

The code-generation algorithm is now the one from Arcade, with a few modifications.
In particular, you can now configure from where the doc comment comes from: It can be the value of the resource, the resx <comment>, or no doc comment at all. This setting defaults to the value for parity with legacy (which plugged the value into a generic "Looks up a string similar to..." template first).
There is still no F# support, nor any support for non-string values.
When/if this PR gets merged, I will create another PR to dotnet/project-system that adds the new metadata into the XAML files so that the user can edit the code-behind generation settings from the IDE Properties panel.
This code no longer checks to see if there is a legacy *.Designer.cs file sitting next to the resx. Since the new algorithm is no longer enabled by default, the onus is now on the developer not to turn on the new generation algorithm where it shouldn't be and cause breakage.

Thanks!


          Correct file name in header comment

9c5d150

Contributor

dasMulli commented Sep 24, 2018

The xsd that drives intellisense for project files is in the msbuild repo at https://github.com/Microsoft/msbuild/blob/master/src/MSBuild/Microsoft.Build.CommonTypes.xsd
though it seems from comments on earlier changes that they also need to manually integrate it into VS after merging it into MSBuild.


          Use correct MSBuild task name

abe714a

wjk added 4 commits

September 29, 2018 15:19


          Update target names to match task name

78e681b


          Fix generation of resx code-behind namespaces

0d04eeb


          Rename metadata to avoid conflict with Arcade

4d3000f


          Improve exception handling

4e41bac

Author

wjk commented Sep 29, 2018

@nguerrera @kant2002 Some help with writing automated unit tests for this functionality would be appreciated. Thanks!


          Remove overriding of resx namespace

ed3ba76

This broke localized resources in sattelite assemblies.

wjk changed the title ~~[WIP] Build time ResX code-behind generation~~ Build time ResX code-behind generation

wjk added 2 commits

September 29, 2018 18:12


          Adjust F# error timing to not fail builds

fbbf1d1

Previously, any usage of a resx files in an F# project would cause
a build error. Now, the error will only occur if the GenerateResxCodeBehind
code is to be invoked.


          Remove unneeded clause from MSBuild conditional

0f894f8

Contributor

nguerrera commented Oct 2, 2018 •

edited

Loading

@wjk I missed some earlier notifications that you were working on this again. Sorry about that.

The general topic of resx-related features is heating up with the coming support for winforms on Core.

For example, this is being looked at actively: dotnet/msbuild#2221

I think in this context, the approach @dasMulli outlined above makes a ton of sense. I believe it may even be possible to use CodeDom now on core.

I would suggest taking this in 3 steps:

Getting msbuild GenerateResource to parity with these inputs: https://github.com/Microsoft/msbuild/blob/8ee3c06d0a1b6a0b424fd69d8eb9222dec4543d1/src/Tasks/Microsoft.Common.CurrentVersion.targets#L2997-L3001
Adding additional features to the strongly-typed generator (such as your DocCommentMode)
Defining nice syntax for resource items to get designer file in obj with appropriate name, etc.

(3) could possibly be for SDK project only, but I think (1) and (2) should be done in msbuild. By sharing the code, we can tackle issues like non-string resources and escaping keyword as they've been tackled before and all in one place.

@rainersigwald What do you think about this approach?

@wjk Thank you for pushing on this. I'm extremely supportive of the user experience this provides, and I just want us to get it done with the right architecture.

Contributor

nguerrera commented Oct 2, 2018

As far as testing, you could do something like

sdk/src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantAllResourcesInSatellite.cs

Line 102 in 506c69f

.HaveStdOutContaining("Hello World from en satellite assembly");

But amend the test project to use generation instead of the manual call to ResourceManager:

sdk/src/Assets/TestProjects/AllResourcesInSatellite/Program.cs

Line 14 in 506c69f

    
           var resources = new ResourceManager("AllResourcesInSatellite.Strings", typeof(Program).GetTypeInfo().Assembly);

That said, you may want to wait for our discussion on where the various pieces should go to land before investing in tests.

nguerrera added the For consideration label

nguerrera modified the milestones: 2.1.3xx, 3.0.1xx

Member

rainersigwald commented Oct 2, 2018

Yes, I think but haven't validated that we can turn the CodeDOM strongly-typed stuff back on for Core now. Since the generation work has been in MSBuild, I agree that that's where it should be now.

wjk mentioned this pull request

Make StronglyTypedResourceGenerator work on .NET Core dotnet/msbuild#3819

Closed

wjk mentioned this pull request

MSBuild generators aren't picked up when using item update mono/monodevelop#6372

Open

Contributor

nguerrera commented Dec 15, 2018

Closing since we're pushing (slowly, sorry for that -- this is a tricky area) forward with the 3 step plan above, starting with dotnet/msbuild#3819

nguerrera closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

For consideration