feat(AIP-160): Add rule to validate filter field name - #1523
Conversation
783060f to
92a0d61
Compare
92a0d61 to
a76ef42
Compare
shwoodard
changed the title
|
|
||
| const ( | ||
| nameMessageFmt = `Guidance: use the field, "string filter", not "string %s"` | ||
| nameSuggestion = "string filter" |
There was a problem hiding this comment.
If this suggestion is applied to the location.DescriptorName it should be simply filter - we don't want to include the type in the suggestion that replaces the FieldDescriptorProto.name
| FieldType string | ||
| FieldName string | ||
| MethodName string | ||
| err bool |
There was a problem hiding this comment.
The typical test pattern is to put problems testutils.Problems{} and just specify the desired finding or nil. I'm guessing this pattern was used to be more DRY?
I'm OK with keeping this as is, but maybe rename from err to invalid or something else since err is so commonly error in Go :)
There was a problem hiding this comment.
This was modeled after TestCount in rules/aip0141/count_suffix_test.go. Nonetheless, good catch! I usually use wantError, but I didn't change it after I copied the aforementioned file.
Unless you feel strongly about including the testutils.Problems{} in the test cases, yes, I think it is more DRY this way.
Changed to wantError.
There was a problem hiding this comment.
wantError is fine with me.
I personally prefer specifying the expected testutils.Problems in each test to avoid putting logic in tests (public TotT), but these tests are so simple I'm not too worried about it. Since we already have the pattern, we'd want to refactor and commit to one or the other wholesale, and one more won't hurt that refactor.
There was a problem hiding this comment.
@noahdietz ftr, I don't know that the Go authors would agree with the TotT or at least that it applies in this scenario. IMO, keeping the repetition caused by having similar tesutils.Problems{} makes the files hard to read either by making the line length long or the table of tests very long. This makes the file harder to navigate. It doesn't seem like logic in the tests, but rather substitution. For these reasons, IMO, at least for table tests, the parameterized solution is preferable.
Validates that the field for filtering is called "filter" and not "filters" for List and custom methods' request messages.
a76ef42 to
5ecb260
Compare
🤖 I have created a release *beep* *boop* --- ## [1.71.0](https://togithub.com/googleapis/api-linter/compare/v1.70.2...v1.71.0) (2025-08-26) ### Features * **AIP-160:** Validate filter field name. ([#1523](https://togithub.com/googleapis/api-linter/issues/1523)) ([73e4f83](https://togithub.com/googleapis/api-linter/commit/73e4f83da2399d9067df2c15310864969a33509f)) * **AIP-160:** Validate filter field type ([#1524](https://togithub.com/googleapis/api-linter/issues/1524)) ([f182a25](https://togithub.com/googleapis/api-linter/commit/f182a25e6ba6ff5c85a7d3300f6d94f19b36d95b)) ### Bug Fixes * **AIP-133:** skip method sig for non-standard create ([#1521](https://togithub.com/googleapis/api-linter/issues/1521)) ([e9306c6](https://togithub.com/googleapis/api-linter/commit/e9306c6f9bd46508fcaefefd3266bdd935c6f2a4)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
2 participants
Validates that the field for filtering is called "filter" and not "filters" for List and custom methods' request messages.