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: various fixes and improvements to schemas #264

Merged
merged 34 commits into from
Jan 13, 2021
Merged

Conversation

wolfy1339
Copy link
Member

No description provided.

@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Jan 11, 2021
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.

Wow these changes look great!

Removes the `changes` property from non-`edit` event actions and completes the properties with information from the docs
@wolfy1339
Copy link
Member Author

@G-Rath If you could review this as well. I've done a lot of changes, just want you to confirm everything will work as intended especially once they get converted to TS types

Copy link
Member

@oscard0m oscard0m left a comment

Choose a reason for hiding this comment

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

This is A-W-E-S-O-M-E! I'm super excited about all the power these changes landing to webhooks. Congrats to everybody 😍

@G-Rath
Copy link
Member

G-Rath commented Jan 12, 2021

@G-Rath If you could review this as well.

Sure thing - I'll try to get to sometime this week (works a bit busy right now, so might have to wait a couple of days)

@wolfy1339
Copy link
Member Author

The openapi spec for the rest api shares a lot of properties with these shemas.
Later on, we could try and use it as the source of truth where possible.

@oscard0m
Copy link
Member

The openapi spec for the rest api shares a lot of properties with these shemas.
Later on, we could try and use it as the source of truth where possible.

Agree on that.

Should we create an issue to follow up this?

@wolfy1339
Copy link
Member Author

Should we create an issue to follow up this?

Yes, for sure.

Copy link
Member

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Overall looks good.

I'll need to do some checking on how the enum: [true] and allOfs come out, but I don't think they'll be any real trouble.

There are a few optimizations you can make, but I've started committing my optimize-schema script which'll handle them automatically.

I'm happy if you want to merge this :)

@wolfy1339
Copy link
Member Author

I'm noticing a lot of events have missing schemas & payload examples for the different actions

@wolfy1339 wolfy1339 merged commit 124b0b2 into master Jan 13, 2021
@wolfy1339 wolfy1339 deleted the update-schemas branch January 13, 2021 22:40
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 3.33.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@oscard0m
Copy link
Member

oscard0m commented Jan 13, 2021

I'm noticing a lot of events have missing schemas & payload examples for the different actions

Should we create an issue for this? Would have sense to tackle it in #266? @wolfy1339 @G-Rath

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants