Update tag.go to accept a context.Context #300
Conversation
| } | ||
|
|
||
| // CreateTag creates a new tag. | ||
| // ListTagsPaginated lists tags on your PagerDuty account, optionally filtered by a search query. | ||
| func (c *Client) ListTagsPaginated(ctx context.Context, o ListTagOptions) ([]*Tag, error) { |
There was a problem hiding this comment.
@stmcallister similar question here as on #297, inspired by #295, as to whether we're okay with this naming convention going forward.
There was a problem hiding this comment.
Now that I'm looking at this naming convention for the third time I'm starting to question it. 😬 Wouldn't it make more sense to call these something like ListAllTags or ListTagsAll because they're gathering up all the pages and presenting a big list? Sorry for not thinking about this earlier.
There was a problem hiding this comment.
@stmcallister I debated this myself for a day or two, and there were a few reasons I ultimately leaned this way:
- It creates a consistent prefix in the name of the method, which should put them near each other in documentation. Helps with discovery.
- Similar to the above, I found it easier to read
<Action><Entity>[<Modifier>]over<Action>[<Modifier>]<Entity>. The optional modifier is at the end and not in the middle. - I want to explore Methods with implicit pagination should accept an include function #296 as part of v2 as a way for consumers to filter the paginated values, which would make the name
Allnot very relevant. It is a 2.0 change so we could rename, but I was thinking less breaking changes the better.
There was a problem hiding this comment.
Ah, okay. Your logic makes sense. Let's stick with the ListTagsPaginated convention. 👍
Updates #267