Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore(pubsub): Add pubsub_list_schema_revisions sample #29174

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aandreassa
Copy link
Contributor

b/266094753

# schema = pubsub.schema "my-schema"
# schema.list_revisions
#
def list_revisions options = {}, view: nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For new methods, please make all "optional" arguments keyword arguments instead of using an "options" hash. The "options" hash pattern is a legacy from older Ruby versions that didn't have good support for keyword arguments.

In particular, it's exceedingly awkward to have an options hash in addition to keyword arguments, because it forces users to do things like this in order to distinguish which keys should go into the options hash vs which should be interpreted as a keyword argument:

my_schema.list_revisions({max: 10, token: "abcde"}, view: "FULL")

For this reason, if an existing method already uses an "options" hash, then we should just add new keys to the existing options. But if a method doesn't already have an "options" hash, we should just add keyword arguments and not include an "options" hash at all.

# * `BASIC` - Include the name and type of the schema, but not the definition.
# * `FULL` - Include all Schema object fields.
#
def list_schema_revisions name, view, options = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, use keyword arguments here instead of an "options" hash.

@dazuma
Copy link
Member

dazuma commented Feb 20, 2025

Also, this adds an actual new method to the client, not just a sample. So it needs to be a feat:.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants