-
Couldn't load subscription status.
- Fork 1.4k
.NET Core resx support #4420
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.
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
.NET Core resx support #4420
Conversation
The previous test setup failed on Linux on runtimes less than 3.0, because of https://github.com/dotnet/corefx/issues/32115. We can't avoid the delay entirely by artificially setting the date into the future, because we also need the output of the next execution of the task to have a later date than the previous one, but we can ensure that the timestamp on the input file is in the future regardless of granularity.
Since the new resx reader works on core, use it to populate the list of linked resources to use in GenerateResource incremental up-to-date checks.
Fixes a possible test-time crash introduced in f8330fd: there's no specified ordering for running the finalizer of _env, so sometimes it might run after another Dispose cleaned up the test environment, and then attempt to throw an assertion exception at a poorly-specified time, leading to a crash in the test process instead of a test failure.
c96d1d4 to
8065d64
Compare
| case "resheader": | ||
| break; | ||
| case "data": | ||
| ParseData(filename, pathsRelativeToBasePath, resources, aliases, elem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: make ParseData return resources, to make it clear what its final goal is.
|
|
||
| private static void ParseData(string resxFilename, bool pathsRelativeToBasePath, List<IResource> resources, Dictionary<string,string> aliases, XElement elem) | ||
| { | ||
| string name = elem.Attribute("name").Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who checks that the xml is in the right resx format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I struggled with going strict or permissive here. The current error handling approach is "assume good faith and throw". I'm optimistic that that's ok since most of these files are machine-generated and machine-edited. If it proves not to be in the previews we can make the errors much nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benvillalobos and I came up with a cheap-but-better-than-it-was improvement in 442c03b. I think that's good enough for 16.3p1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of that catch (Exception). If I followed right, it will lead to confounding user-facing errors like "MSB3103: Invalid Resx file. Object reference not set to an instance of an object.`" when there is actually a bug in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this being good enough for 16.3p1, my experience is that we don't go back and make aggressive catches less aggressive. I'd rather leave proper error handling unfinished than hacked for a preview.
Rewrite tests to be slightly more verbose instead of having a bogus GetHashCode implementation and unexpected public equality.
c5c2f5a to
255de24
Compare
This improves the error experience for invalid resx xml quite a bit, but there's lots of room for further improvement.
442c03b to
221eb77
Compare
Initial implementation of support for non-string resources from
.resxin .NET Core.Uses System.Resources.Extensions (dotnet/corefx#36906) to embed resources in whatever serialization format they were in in the
.resx, deserializing from that format at runtime.Remaining work:
(use this for 16.2, or wait for 16.3/netcore3 feature work? Want this for netcore 3 ASAP)initial deploy in 16.3p1/SDK 3.0.100-p7netcoreapp3.0regardless of MSBuild runtime type: Use preserialized resources to target Core 3 sdk#3342netcoreapp3.0cc @ericstj @nguerrera @merriemcgaw @dreddy-work