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

fix: fix argument and return value handling for get_by_id #1380

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

jgadling
Copy link
Contributor

@jgadling jgadling commented Dec 6, 2024

Fix: #1379

We broke the interface for get_by_id when improving the generated documentation for the client. This fixes the interface and adds a test.

@jgadling jgadling requested review from andy-sweet and uermel December 6, 2024 15:42
@jgadling jgadling changed the title Fix: fix argument and return value handling for get_by_id fix: fix argument and return value handling for get_by_id Dec 6, 2024
Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

This fixes the issue for me and is probably the smallest possible change to fix, so approved. I would personally prefer that the template uses the exact same signature as the base class (i.e. def get_by_id(cls, client, id)), but maybe I'm not following the doc generation code carefully enough.

Copy link
Contributor

@uermel uermel left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix!

@@ -43,14 +43,14 @@ class {{ model.name }}(Model):
find.__func__.__doc__ = Model.find.__func__.__doc__ + find.__func__.__doc__

@classmethod
def get_by_id(cls, **kwargs):
def get_by_id(cls, *args, **kwargs):
"""
Examples:
Get an {{ model.name }} by ID:
>>> {{ model.model_name_underscores }} = {{ model.name }}.get_by_id(client, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not a blocker, but we should find a way to insert class-specific IDs here. The id=1 will return None for almost every important model in the client, so these are not necessarily intuitive examples. I will create a ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Captured here: #1381

@jgadling jgadling merged commit 2504e4d into main Dec 6, 2024
13 of 14 checks passed
@jgadling jgadling deleted the jgadling/fix-get-by-id branch December 6, 2024 16:59
jgadling pushed a commit that referenced this pull request Dec 6, 2024
🤖 I have created a release *beep* *boop*
---


##
[4.2.1](cryoet-data-portal-python-client-v4.2.0...cryoet-data-portal-python-client-v4.2.1)
(2024-12-06)


### 🐞 Bug Fixes

* fix argument and return value handling for get_by_id
([#1380](#1380))
([2504e4d](2504e4d))
* Fix generated examples formatting
([#1369](#1369))
([6cf8473](6cf8473))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model.get_by_id broken in python API client.
3 participants