Skip to content

Conversation

@dxdjgl
Copy link
Contributor

@dxdjgl dxdjgl commented Mar 19, 2023

No description provided.

@dxdjgl
Copy link
Contributor Author

dxdjgl commented Mar 19, 2023

@mgravell This is actually a quite important fix, otherwise all existing usages will have to adapt their project files to include this new option.

@kmosegaard
Copy link
Contributor

kmosegaard commented Mar 19, 2023

Oh, jeez.

I guess, IncludeInOutput is actually by default an empty string (or "") when not specified which bool.TryParse will say is false, causing files not to be generated.

Nice catch and sorry for the blunder.

@kmosegaard
Copy link
Contributor

kmosegaard commented Mar 19, 2023

I tried the following code (using csharp.godbolt.org):

using System;

class Program
{
    static void Main()
    {
        bool result = true;
        var res = bool.TryParse("", out result);
        Console.WriteLine(res);
        Console.WriteLine(result);
    }
}

It seems that both res is false and result is false. So, bool.TryParse must also have overwritten result even though it couldn't be parsed. So, we might have to refine the logic even more.

This is actually documented behavior: https://learn.microsoft.com/en-us/dotnet/api/system.boolean.tryparse?view=net-7.0.

We might have to refine the whole thing to be something like:

var includeInOutput = true;
if (userOptions is not null && userOptions.TryGetValue(Literals.AdditionalFileMetadataPrefix + "IncludeInOutput", out var optionValue) && !bool.TryParse(optionValue, out var includeInOutput))
{
    includeInOutput = true;
}

Other suggestions?

@kmosegaard
Copy link
Contributor

Would be nice if we also extend ProtoGeneratorTests.cs specifically L88 as such:

}", new[] { ("ImportPaths", "../../"), ("IncludeInOutput", "") } ),

With an empty IncludeInOutput to see that the change works.

@mgravell mgravell merged commit e719cc7 into protobuf-net:main Mar 19, 2023
@mgravell
Copy link
Member

Merged to avoid problems, but I might tweak it again shortly - a useful trick is to use a temp for the TryBlah out variable

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.

3 participants