Skip to content

Commit

Permalink
Only apply 'field.id.{negative,zero}' to explicit field IDs (#50)
Browse files Browse the repository at this point in the history
Auto-assigned field IDs start at zero and then go negative. If auto-
assigned fields are intentionally used (i.e. no `field.id.missing`
check), then it's not helpful to report additional errors about the
range of the auto-assigned values.

These checks now only report errors for explicitly assigned field IDs.
  • Loading branch information
jparise authored Apr 28, 2022
1 parent 80432a2 commit 18eb871
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 8 deletions.
8 changes: 4 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ implicit/auto-assigning syntax).

### `field.id.negative`

This check reports an error if a field's ID is negative.
This check reports an error if a field's ID is explicitly negative.

### `field.id.zero`

This check reports an error if a field's ID is zero, which is generally
unsupported by the Apache Thrift compiler. This is distinct from the
`field.id.negative` check given the existence of the `--allow-neg-keys`
This check reports an error if a field's ID is explicitly zero, which is
generally unsupported by the Apache Thrift compiler. This is distinct from
the `field.id.negative` check given the existence of the `--allow-neg-keys`
Apache Thrift compiler option.

### `field.optional`
Expand Down
8 changes: 4 additions & 4 deletions checks/fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,19 @@ func CheckFieldIDMissing() *thriftcheck.Check {
})
}

// CheckFieldIDNegative reports an error if a field's ID is negative.
// CheckFieldIDNegative reports an error if a field's ID is explicitly negative.
func CheckFieldIDNegative() *thriftcheck.Check {
return thriftcheck.NewCheck("field.id.negative", func(c *thriftcheck.C, f *ast.Field) {
if f.ID < 0 {
if !f.IDUnset && f.ID < 0 {
c.Errorf(f, "field ID for %q (%d) is negative", f.Name, f.ID)
}
})
}

// CheckFieldIDZero reports an error if a field's ID is zero.
// CheckFieldIDZero reports an error if a field's ID is explicitly zero.
func CheckFieldIDZero() *thriftcheck.Check {
return thriftcheck.NewCheck("field.id.zero", func(c *thriftcheck.C, f *ast.Field) {
if f.ID == 0 {
if !f.IDUnset && f.ID == 0 {
c.Errorf(f, "field ID for %q is zero", f.Name)
}
})
Expand Down
8 changes: 8 additions & 0 deletions checks/fields_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ func TestCheckFieldIDNegative(t *testing.T) {
`t.thrift:0:1: error: field ID for "Field" (-1) is negative (field.id.negative)`,
},
},
{
node: &ast.Field{IDUnset: true},
want: []string{},
},
}

check := checks.CheckFieldIDNegative()
Expand All @@ -77,6 +81,10 @@ func TestCheckFieldIDZero(t *testing.T) {
node: &ast.Field{ID: -1, Name: "Field"},
want: []string{},
},
{
node: &ast.Field{IDUnset: true},
want: []string{},
},
}

check := checks.CheckFieldIDZero()
Expand Down

0 comments on commit 18eb871

Please sign in to comment.