-
Notifications
You must be signed in to change notification settings - Fork 2
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
Reflect recent API changes in event and player endpoints #2
Conversation
maxim-01
commented
Nov 28, 2023
•
edited
Loading
edited
- Updated ban-related fields in line with recent changes
- Narrowed down fields in certain types for better IntelliSense
- Loosened param types for methods returning API routes to accept string where applicable
- Updated ban-related fields in line with recent changes - Narrowed down fields in certain types for better IntelliSense - Loosened param types for methods returning API routes to accept string here applicable
Thank you very much for a pull request with so many improvements! I will review it in the following days. I let CI run to trigger the ESLint validation. Can you please take a look at it and make changes so it follows the setup rules of the repository? Also, I am not the biggest fan of the pull request title; do you think you can come up with something more descriptive? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, I am very sorry for the delay in getting to this pull request. As these repositories are not in my main scope, I have to find some extra time to properly maintain them.
The changes look really nice overall. Thank you a lot for this pull request; I can definitely see a huge improvement. I have left a few comments regarding my concerns and requests for little changes.
- removed exported `APIPlayerBans` type - removed string from argument types when numeric identifiers are expected - merged types that did not require separation
There was a problem hiding this 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 changes! I left a few little remarks before we can merge this.
- generalised `as const` where applicable - renamed the base interface for `APIPlayer` to align with `APIPlayerPatreon`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Thank you for the fast iteration! And thank you once again for this extensive pull request improving the API types. 🚀
I am currently outside the town, so I will make a release later. Just so you know: we will also be moving to the @truckersmp
organization (instead of @truckersmp_official
) because we finally got the name from npm after months of waiting.
Thank you once again for this pull request. This change has been released as a part of v1.1.0. |