Cleanup various bits of Google.Protobuf #6674
Conversation
|
Could you possibly review this @jtattermusch? |
|
@ObsidianMinor there's a conflict |
Remove IsInitialized checks accidentally left in MessageParser Simplify ExtensionCollection.CrossLink
ObsidianMinor
force-pushed
the
csharp/general-cleanup
branch
from
73f3185
to
71c492d
Compare
|
Fixed |
|
Is this purely cosmetic or does it actually fix any user-visible bugs? |
| declarationOrder.Add(descriptor.ExtendeeType, new List<FieldDescriptor>()); | ||
| } | ||
| IList<FieldDescriptor> list; | ||
| if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out list)) |
There was a problem hiding this comment.
- in protobuf codebase, always use curly brackets for
ifstatements (single statements ifs and loops are considered dangerous). - I think
list = new List<FieldDescriptor>();
declarationOrder.Add(......., list);
is more readable.
There was a problem hiding this comment.
Could we add an editorconfig with csharp_prefer_braces enabled to prevent style issues like this and others in the future? In another PR of course.
There was a problem hiding this comment.
At some point, I'd like to add a set of rules to enforce code style in the protobuf C# codebase. I'm not sure what the best approach is (there are multiple ways of doing this).
| @@ -261,6 +261,16 @@ public void RequiredFields() | |||
| Assert.True(message.IsInitialized()); | |||
| } | |||
|
|
|||
| // Code was accidentally left in message parser that threw exceptions when missing required fields after parsing. | |||
There was a problem hiding this comment.
thanks for adding the comment.
nit: Use /// <summary> block for comments outside the method body.
While adding nullable annotations throughout the lib, I found a few missing null-checks and some IsInitialized checks accidentally left in MessageParser. Additionally, ExtensionCollection.CrossLink did some unnecessary things so this simplifies that.