-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Restore write-resource-to-text tests #4830
Conversation
Closes dotnet#308 by re-enabling the tests.
I thought this worked on my machine but I was testing desktop framework. Sigh. Pulling for now. |
This was working for ResXResourceReader-based resources but not the new reader.
There are various failures possible in ReadResources, but the existing test applies only to full framework
Uses the existing category from Xunit.NetCore.Extensions
These situations logged confusing English-only errors; now they fail and the error can get localized.
af395fb
to
60f260c
Compare
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.
Looks good to me. I had just a few questions.
@@ -2988,7 +2994,7 @@ private void ReadResources(String filename, bool shouldUseSourcePath, String out | |||
#if FEATURE_ASSEMBLY_LOADFROM | |||
ReadAssemblyResources(filename, outFileOrDir); | |||
#else | |||
_logger.LogError("Reading resources from Assembly not supported on .NET Core MSBuild"); | |||
throw new InputFormatNotSupportedException("Reading resources from Assembly not supported on .NET Core MSBuild"); |
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.
Why is it advantageous to throw instead of just logging an error? Are you trying to reduce duplicate output?
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.
Logging an error here is nonfatal, so the experience with going through this codepath was:
- This error.
- Some other odd output when trying to move on.
- Another fatal error that didn't directly indicate the problem.
Since there were already exception-based error paths in ProcessFile
I opted to add another rather than change everything to error returns.
{ | ||
if (assemblyName == null) |
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.
Why would they be null?
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.
That's what happens if an assembly name can't be extracted from a reference for some reason in PopulateAssemblyNames()
. The other codepaths were robust to this situation but this newly-added one wasn't (until running a test revealed the error).
namespace Microsoft.Build.Tasks.ResourceHandling | ||
{ | ||
[Serializable] | ||
internal class InputFormatNotSupportedException : Exception |
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.
Is there a reason you can't use FormatException
or ArgumentException
instead of making your own (empty) new type of exception?
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.
Having a custom exception type makes detecting this error condition easy using a catch-specific-type block.
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.
Tracing how it's currently caught, I believe a FormatException will still have the exact same effect.
Also, I don't see anywhere that you try to catch it; do you have plans for adding something like that in the future? If not, you're breaking something that could otherwise be caught by an ArgumentException, failing more cleanly.
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 believe a FormatException will still have the exact same effect.
Since it's a standard exception type, other code could insert a FormatException
that would then be inadvertently caught and handled as this specific problem, rather than handled at another layer.
Also, I don't see anywhere that you try to catch it
If not, you're breaking something that could otherwise be caught by an ArgumentException, failing more cleanly.
This is harder to distinguish since other places could throw ArgumentException
inside the big ProcessFile
catch block.
@@ -979,6 +979,11 @@ | |||
<target state="translated">MSB3190: ClickOnce no admite el nivel de ejecución de solicitudes '{0}'.</target> | |||
<note>{StrBegin="MSB3190: "}</note> | |||
</trans-unit> | |||
<trans-unit id="GenerateResource.CoreSupportsLimitedScenarios"> | |||
<source>MSB3824: In order to build with .NET Core, resource inputs must be in .txt or .resx format.</source> |
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 guessing this is supposed to be a localized version. When does that happen?
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.
Correct. The loc team periodically (at least once per minor release) does a pass on strings and will submit a PR with updated localizations (#4679 was a recent one). Until that's done the English copy of the string is used.
Docs are here: https://github.com/microsoft/msbuild/blob/master/documentation/wiki/Localization.md
if (string.IsNullOrEmpty(resgenFile)) | ||
{ | ||
resgenFile = GetTempFileName(".resx"); | ||
File.Delete(resgenFile); |
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) Is this line necessary? According to the File.WriteAllText documentation, even if the file did exist it would be overwritten
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.
It's preexisting but no, I don't think it is. I'll remove.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Closes #308 by re-enabling the tests.