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).
| 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.
jtattermusch
left a comment
jtattermusch
left a comment
There was a problem hiding this comment.
LGTM after changing the comment style to <summary>.
5 participants
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.