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

Repairing two 204-related serde errors, and adding some simple tests. #503

Merged
merged 4 commits into from
Jan 13, 2024

Conversation

manchicken
Copy link
Contributor

@manchicken manchicken commented Dec 15, 2023

Fixed

  • Fixed two functions which return 204s and have no body, which throws serde errors
    • octocrab.issues().delete_label()
    • octocrab.teams().repos().remove()

Other

  • Added tests for the above functions
  • Also added tests for octocrab.teams().repos().add_or_update()
  • Also added tests for octocrab.issues().remove_label()

This PR is for issue #504.

- Adding a custom FromResponse handler for 204s
- It uses the `octocrab::_delete()` function, though, which seems like a bad idea.
- The tests do all pass, including the one I added, though.

Feedback and other ideas are welcome. In general, I think we need a
new approach to the 204 responses. I kinda think the `delete()` function
needs a minor re-factor, but I worry that that might have
some breaking changes and common-law features.
@manchicken manchicken changed the title Adding a test which shows the error. Repairing two 204-related serde errors, and adding some simple tests. Jan 13, 2024
@manchicken manchicken marked this pull request as ready for review January 13, 2024 04:16
@manchicken
Copy link
Contributor Author

@XAMPPRocky, I think this is ready for review.

@XAMPPRocky XAMPPRocky merged commit 69c7869 into XAMPPRocky:main Jan 13, 2024
10 checks passed
@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@github-actions github-actions bot mentioned this pull request Jan 13, 2024
@manchicken
Copy link
Contributor Author

Thanks for the quick merge!

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.

2 participants