Fixed FormatException when trying to publicize the Photon assemblies in R.E.P.O.

[AraHaan]

Refactor TryGetMetadata method to handle missing metadata correctly and improve comments regarding Photon assemblies.

[AraHaan]

cc: @js6pak can we please get this in ASAP, this is a blocking issue for my R.E.P.O. mod that uses BepInEx.

[toebeann]

If you can fix it locally, you can reference it locally, and use it in your project without requiring the PR to be merged ASAP.

[AraHaan]

If you can fix it locally, you can reference it locally, and use it in your project without requiring the PR to be merged ASAP.

I fixed this in the GitHub web editor 😂.

[toebeann]

If you can fix it locally, you can reference it locally, and use it in your project without requiring the PR to be merged ASAP.

I fixed this in the GitHub web editor 😂

And you can't clone your PR locally and use it?

[arrowmaster]

My C# knowledge isn't much but logically this looks confusing after the change because of the duplicate metadata = null; return false; in the decleration. Wouldn't this be better achieved by just wrapping the return true; with the string.IsNullOrWhiteSpace(metadata) check?

I fixed this in the GitHub web editor 😂.

So this is untested?

[Lordfirespeed]

Here is the fix I originally suggested before the PR author edited it

public static bool TryGetMetadata(this ITaskItem taskItem, string metadataName, [NotNullWhen(true)] out string? metadata)
{
    metadata = null;
    if (!taskItem.HasMetadata(metadataName)) return false;
    metadata = taskItem.GetMetadata(metadataName);
    if (!String.IsNullOrWhiteSpace(metadata)) return true;
    metadata = null;
    return false;
}

This is untested but it doesn't need testing. It's verifiably the same behaviour as the original function, with the caveat that it returns false and sets metadata to null for whitespace values

I can't necessarily say the same for this PR's changes
Edit: PR changes seem to be nearly identical to this with some changes to braces, so same goes for the PR changes.

[AraHaan]

I will need to apply suggested changes tomorrow as my PC's power supply decided today of all days that it was time for it to quit working entirely and the replacement might arrive then.

[Lordfirespeed]

My C# knowledge isn't much but logically this looks confusing after the change because of the duplicate metadata = null; return false; in the decleration. Wouldn't this be better achieved by just wrapping the return true; with the string.IsNullOrWhiteSpace(metadata) check?

When metadata is a whitespace string the modified function returns false to indicate failure. the caller expects that metadata is null when the function returns false, so the value has to be explicitly reset to null
it's not a duplicate declaration, it's just a variable reassignment

[arrowmaster]

When metadata is a whitespace string the modified function returns false to indicate failure. the caller expects that metadata is null when the function returns false, so the value has to be explicitly reset to null
it's not a duplicate declaration, it's just a variable reassignment

The "variable reassignment" is a duplication of work, splitting the logic by having two sections doing the same metadata = null; return false; which I personally find harder to read than the original.

This is what I was thinking, which would just be a three line addition and follows the original style for readability.

public static bool TryGetMetadata(this ITaskItem taskItem, string metadataName, [NotNullWhen(true)] out string? metadata)
{
    if (taskItem.HasMetadata(metadataName))
    {
        metadata = taskItem.GetMetadata(metadataName);
        if (!string.IsNullOrWhiteSpace(metadata))
        {
            return true;
        }
    }

    metadata = null;
    return false;
}

[Lordfirespeed]

@arrowmaster i dislike this because imo nesting ifs is evil.
Say another if should be added to this function, it gets less and less readable each time in your method.
in the proposed layout more conditions can be added without degrading readability.

I don't care to argue over semantics/codestyle any further, the code-owners/maintainers should express their preference.

[AraHaan]

I agree with them on this, nesting ifs becomes a complex and unreadable problem at some point.

[Hamunii]

I just realized that this is the fix for the Publicize thingy being broken:

<Publicize Include="Foo" />

I didn't even realize that because this is in no way related to Photon, and as I was going to fix this issue myself, I realized that what I need to do is exactly what this PR does.

Anyways, I want this fixed so I can use the <Publicize Include="Foo" /> thingy in my mod template, without including a lot of boilerplate to explicitly define the values that are implicitly set to empty string instead of null.