Skip to content

Fix identifiers generated from resx resource names#2208

Merged
tmat merged 3 commits into
dotnet:masterfrom
tmat:ResxFixes
Jan 2, 2020
Merged

Fix identifiers generated from resx resource names#2208
tmat merged 3 commits into
dotnet:masterfrom
tmat:ResxFixes

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Mar 10, 2019

No description provided.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Mar 10, 2019

@ericstj

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Mar 11, 2019

We intentionally didn't do this for CoreFx because we want folks to use resource IDs that are valid identifiers rather than have some transformation that has to be reversed when translating between identifier and resource ID. Does someone have a valid scenario for a resource ID that must be illegal identifier syntax?

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Mar 11, 2019

The problem is that we have tons of existing resources. Switching Roslyn from the default resource generation to Arcade's produced bad source files. Some resource names used in Roslyn have . in the names.

Alternatively we could report error and change the resx files. Then we would need pretty much the same logic.

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Mar 11, 2019

I see. IMHO I'd prefer an error, that way you let a human choose the identifier they want in their source, but I don't feel too strongly about it.

[InlineData("A\u0660\u2040\u0601\u0300\u0903", "A\u0660\u2040\u0601\u0300\u0903")]
public void GetIdentifierFromResourceName(string name, string expectedIdentifier)
{
Assert.Equal(expectedIdentifier, GenerateResxSource.GetIdentifierFromResourceName(name));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing to build: missing internals visible to?

@jonfortescue
Copy link
Copy Markdown
Contributor

@tmat -- what is the status on this?

@tmat
Copy link
Copy Markdown
Member Author

tmat commented May 17, 2019

I don't have bandwidth to finish this right now. Will get back to it later.

@jonfortescue
Copy link
Copy Markdown
Contributor

@tmat -- just poking again to see how this is going. Do you think you'll be picking this back up soon? If not, I think we probably ought to close it.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jul 11, 2019

Keep it open please.

@mmitche
Copy link
Copy Markdown
Member

mmitche commented Nov 13, 2019

@tmat Should we keep this open?

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Nov 13, 2019

Yes please. I'll get back to it some day.

@tmat tmat merged commit 7af51d4 into dotnet:master Jan 2, 2020
@tmat tmat deleted the ResxFixes branch January 2, 2020 16:07
Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/GenerateResxSource.cs
@rladuca
Copy link
Copy Markdown
Member

rladuca commented Jan 8, 2020

@tmat WPF seems broken by this (See https://dev.azure.com/dnceng/public/_build/results?buildId=475850&view=results)

Our generated resource files have bad const string lines like this:

/// <summary>Value of {0} is outside of the allowed range.</summary>
internal const string @ArgumentOutOfRange = "ArgumentOutOfRange");
/// <summary>The value of {0} cannot be changed while the inertia processor is running.</summary>
internal const string @CannotChangeParameterDuringInertia = "CannotChangeParameterDuringInertia");

FYI @vatsan-madhavan

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Jan 9, 2020

@tmat seems like missed removing trailing parentheses here:

strings.AppendLine($"{memberIndent}internal const string @{identifier} = \"{name}\");");

I'll create a PR to fix this.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jan 9, 2020

Yes, my bad! I tested my change and then I did one "minor" tweak and did not test it again - because it was "just" a minor tweak :-|

agocke pushed a commit that referenced this pull request Mar 28, 2020
* Fix identifiers generated from resx resource names

* Make AggressiveInlining NET20 conditional

(cherry picked from commit 7af51d4)
mmitche pushed a commit that referenced this pull request Mar 30, 2020
* Fix identifiers generated from resx resource names

* Make AggressiveInlining NET20 conditional

(cherry picked from commit 7af51d4)

Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants