Skip to content
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

style guide: make enum and union fields snake_case the same as struct fields #2101

Open
andrewrk opened this issue Mar 24, 2019 · 7 comments
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. docs proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Mar 24, 2019

Currently we have this problem with union(enum):

zig/std/zig/ast.zig

Lines 110 to 130 in aff7b38

pub const Error = union(enum) {
InvalidToken: InvalidToken,
ExpectedVarDeclOrFn: ExpectedVarDeclOrFn,
ExpectedVarDecl: ExpectedVarDecl,
ExpectedAggregateKw: ExpectedAggregateKw,
UnattachedDocComment: UnattachedDocComment,
ExpectedEqOrSemi: ExpectedEqOrSemi,
ExpectedSemiOrLBrace: ExpectedSemiOrLBrace,
ExpectedColonOrRParen: ExpectedColonOrRParen,
ExpectedLabelable: ExpectedLabelable,
ExpectedInlinable: ExpectedInlinable,
ExpectedAsmOutputReturnOrType: ExpectedAsmOutputReturnOrType,
ExpectedCall: ExpectedCall,
ExpectedCallOrFnProto: ExpectedCallOrFnProto,
ExpectedSliceOrRBracket: ExpectedSliceOrRBracket,
ExtraAlignQualifier: ExtraAlignQualifier,
ExtraConstQualifier: ExtraConstQualifier,
ExtraVolatileQualifier: ExtraVolatileQualifier,
ExpectedPrimaryExpr: ExpectedPrimaryExpr,
ExpectedToken: ExpectedToken,
ExpectedCommaOrEnd: ExpectedCommaOrEnd,

Now, if you use Error.InvalidToken are you referring to the type, or the enum value? It's ambiguous. This causes switch expressions to resort to awkward uses of @TagType:

zig/std/zig/ast.zig

Lines 133 to 141 in aff7b38

switch (self.*) {
// TODO https://github.com/ziglang/zig/issues/683
@TagType(Error).InvalidToken => |*x| return x.render(tokens, stream),
@TagType(Error).ExpectedVarDeclOrFn => |*x| return x.render(tokens, stream),
@TagType(Error).ExpectedVarDecl => |*x| return x.render(tokens, stream),
@TagType(Error).ExpectedAggregateKw => |*x| return x.render(tokens, stream),
@TagType(Error).UnattachedDocComment => |*x| return x.render(tokens, stream),
@TagType(Error).ExpectedEqOrSemi => |*x| return x.render(tokens, stream),
@TagType(Error).ExpectedSemiOrLBrace => |*x| return x.render(tokens, stream),

However if we had a different style guide, the enum value would be Error.invalid_token and the type would be Error.InvalidToken.

This is less of a problem now that #683 is mostly solved, but there is still the ambiguity.

This sort of conflicts with #995.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Mar 24, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Mar 24, 2019
@Akuli
Copy link
Contributor

Akuli commented Mar 26, 2019

How about UPPER_CASE for enum fields? Many other languages do that too.

@andrewrk
Copy link
Member Author

andrewrk commented Apr 30, 2019

I'll note that @thejoshwolfe even before this proposal already broke from the style guide to do this: https://github.com/thejoshwolfe/legend-of-swarkland/blob/ace37864bf4754851bfe0c5256077171c2db695f/src-core/protocol.zig#L8-L28

I'll also note that the enums in @import("builtin") adhere to this, but that's more for consistency with the LLVM names.

@thejoshwolfe
Copy link
Sponsor Contributor

for unions specifically, the options are very similar to fields, so matching the style of field names makes more sense than matching the style of type names. For enums, the options are most similar to global constants, and i believe the style there is snake case as well. For union enums, we have to choose one style for both.

In no way does it make sense for the options to match the style of a type name.

@andrewrk andrewrk added accepted This proposal is planned. and removed accepted This proposal is planned. labels May 1, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 20, 2019
@andrewrk andrewrk added accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. labels Dec 31, 2019
@andrewrk
Copy link
Member Author

After months of silent deliberation and sleeping on it, this is now accepted.

I'm in no hurry to break everybody's code, but this is the new style for the standard library.

Exceptions:

  • If there is some pre-existing naming convention, use that instead.
  • If there are Zig language keywords desired to be used in the enum, using capitalization to work around them would be reasonable.
  • Projects and applications are generally encouraged to use whatever style they prefer; the style guide is provided as a "tie-breaker" in order to provide a default when there is ambivalence.

@andrewrk andrewrk added docs standard library This issue involves writing Zig code for the standard library. labels Dec 31, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Jan 3, 2020
@hryx
Copy link
Sponsor Contributor

hryx commented Jan 5, 2020

Will this style be applied to errors too? (Tangentially related: #2647)

How about UPPER_CASE for enum fields?

@Akuli I think that would be the right thing to do if the compiler style guide already had that e.g. for constants, but as of now it doesn't use that style for naming anywhere.

@andrewrk
Copy link
Member Author

andrewrk commented Jan 5, 2020

Will this style be applied to errors too? (Tangentially related: #2647)

No, I think errors can remain as is.

@SebastianKeller
Copy link
Contributor

SebastianKeller commented Jul 14, 2020

One thing to consider is the null keyword.

The following example wont compile:

pub const Value = union(enum) {
    null: void,
    bool: bool,
    int: i64,
    uint: u64,
    float: f64,
    string: []const u8,
    object: std.StringHashMap(Value),
};
./types.zig:174:5: error: expected token '}', found 'null'
    null: void,
    ^

If i escape null it works just fine. But then again, the access is quite odd.

pub const Value = union(enum) {
    @"null": void,
    bool: bool,
    int: i64,
    uint: u64,
    float: f64,
    string: []const u8,
    object: std.StringHashMap(Value),
};

This issue applies to std.json.Value.

ifreund added a commit to ifreund/ziglearn that referenced this issue Oct 14, 2020
ifreund added a commit to ifreund/zig that referenced this issue Oct 14, 2020
This follows the accepted change to the style guide:
ziglang#2101
Vexu pushed a commit that referenced this issue Oct 15, 2020
This follows the accepted change to the style guide:
#2101
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. docs proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

5 participants