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

feat(types): use OpenAPI spec #841

Merged
merged 33 commits into from
Feb 11, 2024
Merged

feat(types): use OpenAPI spec #841

merged 33 commits into from
Feb 11, 2024

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Apr 15, 2023

Resolves #840


Behavior

Before the change?

  • The types were made in house by using example payloads and extrapolated a schema from them

After the change?

  • The types use the official GitHub OpenAPI spec

Other information


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • Since we are switching the source of the types, I feel it better to release this as a breaking change

Pull request type

Please add the corresponding label for change this PR introduces:

  • Feature/model/API additions: Type: Feature

@wolfy1339 wolfy1339 added Type: Feature New feature or request Type: Breaking change Used to note any change that requires a major version bump labels Apr 15, 2023
@ollie-iterators
Copy link

ollie-iterators commented Apr 30, 2023

Some errors:

Error: test/typescript-validate.ts(106,7): error TS2578: Unused '@ts-expect-error' directive.

Also some from the types.d.ts about the property "webhook-check-run-created-form-encoded" not existing on a type

@wolfy1339
Copy link
Member Author

I am aware of the errors.

Error: test/typescript-validate.ts(106,7): error TS2578: Unused '@ts-expect-error' directive.

This is due to github/rest-api-description#2465
The test is supposed to error in normal circumstances, and the error is ignored.

Also some from the types.d.ts about the property "webhook-check-run-created-form-encoded" not existing on a type

That has been fixed. octokit/openapi-webhooks@01457e3

I haven't updated this PR

@ollie-iterators
Copy link

Great

@wolfy1339 wolfy1339 linked an issue Aug 25, 2023 that may be closed by this pull request
@wolfy1339 wolfy1339 added the project Goes beyond a single bug fix or new feature. Help welcome by existing contributors. label Nov 19, 2023
@wolfy1339
Copy link
Member Author

The webhooks OpenAPI spec has been out for a year now.

The schemas (excluding enum values) are pretty much the same. I have noticed in many areas where they don't change the enum according to the action.

Overall it would be nice to have people test this package with the OpenAPI webhooks spec

@wolfy1339 wolfy1339 marked this pull request as ready for review November 19, 2023 21:07
@wolfy1339
Copy link
Member Author

@oscard0m @gr2m @G-Rath I'd like to get your input on this

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I looked through the changes and it looks great, thank you for putting in all this work! When we ship it, shall we move @wolfy1339/openapi-webhooks-types and @wolfy1339/openapi-webhooks to @octokit as well?

@wolfy1339
Copy link
Member Author

I looked through the changes and it looks great, thank you for putting in all this work! When we ship it, shall we move @wolfy1339/openapi-webhooks-types and @wolfy1339/openapi-webhooks to @octokit as well?

Yes, that was the goal

@wolfy1339
Copy link
Member Author

I am coming around to this PR again, I have created octokit/openapi-webhooks#80 to track the move to the Octokit org.

I don't have the permissions to move the repo myself, so whomever is available that can do it @kfcampbell @gr2m @nickfloyd

@wolfy1339 wolfy1339 changed the title 🚧 PoC: use OpenAPI spec feat(types): use OpenAPI spec Feb 11, 2024
@wolfy1339 wolfy1339 merged commit 1168fa6 into beta Feb 11, 2024
3 checks passed
@wolfy1339 wolfy1339 deleted the openapi branch February 11, 2024 13:19
Copy link
Contributor

🎉 This PR is included in version 12.2.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project Goes beyond a single bug fix or new feature. Help welcome by existing contributors. released on @beta Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Adapt the types to the OpenAPI spec types
3 participants