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: Various API model fixes #403

Merged
merged 6 commits into from
Apr 21, 2024
Merged

fix: Various API model fixes #403

merged 6 commits into from
Apr 21, 2024

Conversation

tazlin
Copy link
Member

@tazlin tazlin commented Apr 19, 2024

In the course of expanding the SDK, certain inconsistencies were made apparent. This PR serves to correct the issues.

  • fix: use SinglePeriodTxtStat for text totals stat endpoint
    • Previously, probably due to copy and pasting, the /v2/stats/text/totals endpoint published in the swagger doc that returned elements were of type SinglePeriodImgStat, which could be a source of confusion when consuming the swagger doc. /v2/stats/img/totals returns elements with different attributes, so this PR hopefully will clarify that the two endpoints return two different types of objects.
  • fix: set min_length for image attr in ExtraSourceImage
    • This accurately reflects that it is a required field for creating a ExtraSourceImage
  • fix: include customizer on user info endpoints
  • fix: match PUT horde mode arg names to ret. payload
    • The input parameters names did not match the returned fields for the PUT action of /v2/status/modes. For clarity (and ease of object/model reuse client-side) I am changing the payload argument names to match. This is a breaking change, but as only admins (i.e., you) can use it, I think it is safe to do so.
  • fix: match apidoc for ModelSingle to actual return type
    • While this endpoint should return a single value, in reality it does (and probably always has) return a list of a single element instead. Because changing the actual return value constitutes a breaking change, I recommend simply fixing the API doc for now and changing it to the (presumably) intended type when we go to /v3/.
  • ci: allow SDK async tests to run
    • Forthcoming AI-Horde CI SDK-based tests will require pytest-asyncio as a reqs.dev.txt dependency.

Previously, probably due to copy and pasting, the `/v2/stats/text/totals` endpoint published in the swagger doc that returned elements were of type `SinglePeriodImgStat`, which could be a source of confusion when consuming the swagger doc.
@tazlin tazlin added the allow-ci A PR with this label will run through CI. label Apr 19, 2024
@tazlin tazlin marked this pull request as draft April 19, 2024 13:56
This accurately reflects that it is a required field for creating a `ExtraSourceImage`
@tazlin tazlin changed the title fix: use SinglePeriodTxtStat for text totals stat endpoint fix: use SinglePeriodTxtStat for text totals stat endpoint; fix: min_length for ExtraSourceImage Apr 19, 2024
@tazlin tazlin changed the title fix: use SinglePeriodTxtStat for text totals stat endpoint; fix: min_length for ExtraSourceImage fix: Various API model fixes Apr 19, 2024
@db0
Copy link
Member

db0 commented Apr 20, 2024

LGTM until now

tazlin added 3 commits April 20, 2024 13:00
While this endpoint *should* return a single value, in reality it does (and probably always has) actually return a list of a single element instead. Because changing the actual return value constitutes a breaking change, I recommend simply fixing the API doc for now and changing it to the (presumably) intended type when we go to `/v3/`.
This missing dependency prevents a good portion of the network-based and API-call-based SDK tests from running.
@tazlin tazlin marked this pull request as ready for review April 20, 2024 23:30
@tazlin
Copy link
Member Author

tazlin commented Apr 20, 2024

@db0 Please review these changes at your convenience. Nothing should be a breaking change but you if you have any concerns please feel free to raise them. Otherwise, please merge and deploy when you are willing. A pending SDK change would benefit in testing against production with the changes in this PR.

@tazlin tazlin requested a review from db0 April 21, 2024 10:54
@db0 db0 merged commit 1b6b631 into main Apr 21, 2024
2 checks passed
@db0 db0 deleted the SinglePeriodTxtStat branch April 22, 2024 08:51
tazlin added a commit to Haidra-Org/horde-sdk that referenced this pull request May 9, 2024
tazlin added a commit to Haidra-Org/horde-sdk that referenced this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-ci A PR with this label will run through CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants