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

Allow searching with company slug #37

Conversation

iDiegoNL
Copy link
Contributor

Currently, it already is a possibility to search by company slug in the API. E.g. https://api.truckersmp.com/v2/vtc/phoenix. However, because of the type declarations in this package code, this wasn't an option here yet.

This PR resolves that issue, including a test that checks the functionality with a company slug.

Sadly, because this package isn't PHP ^8.0, declaring the property types as int|string wasn't possible. However, declaring them as a string shouldn't be a problem because of type juggling. And they are declared as string|int in the DocBlocks.

This should all work as expected, with both slugs and integer IDs, but let me know if there is any feedback.

@ShawnCZek ShawnCZek self-assigned this Nov 10, 2021
Copy link
Member

@ShawnCZek ShawnCZek left a comment

Choose a reason for hiding this comment

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

The comments and variable names should probably suggest that fetching by the slug is also possible. Otherwise, it all seems to be okay. 👍

@iDiegoNL
Copy link
Contributor Author

@ShawnCZek what's your opinion on renaming that $id var to $idOrSlug? This will make it even clearer to the end users then 😄

@ShawnCZek
Copy link
Member

@iDiegoNL I was rather thinking of something like $key and then explaining it in the docs.

@iDiegoNL
Copy link
Contributor Author

iDiegoNL commented Nov 11, 2021

@iDiegoNL I was rather thinking of something like $key and then explaining it in the docs.

Interesting, but if someone needs to read the documentation in order to fully understand what a variable can be & means, it's not a good name in my opinion :)

@ShawnCZek
Copy link
Member

@iDiegoNL I was just thinking ahead of the possibility of adding another "key" for the search.
As otherwise, in the future, it would have to be $idOrNameOrWhatever.

@iDiegoNL
Copy link
Contributor Author

@ShawnCZek That is true indeed, could get messy if we were to add more "searchable properties". Good shout 😆

With "adding another", do you mean adding a $key var as an addition to the $id var, and if so, can you provide an example/description of how you'd like to see this? Or do you simply mean renaming the $id var, because that indeed doesn't seem like a bad option after your explanation :)

@ShawnCZek
Copy link
Member

@iDiegoNL Just renaming the variable and modifying the documentation to mention both the options.

@iDiegoNL
Copy link
Contributor Author

@ShawnCZek above suggestion has been implemented 🙂

Copy link
Member

@ShawnCZek ShawnCZek left a comment

Choose a reason for hiding this comment

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

It is all looking great now. Thank you for this pull request!

@ShawnCZek ShawnCZek merged commit a8c1e14 into TruckersMP:develop Nov 13, 2021
ShawnCZek added a commit that referenced this pull request Nov 13, 2021
iDiegoNL added a commit to iDiegoNL/API-Client that referenced this pull request Nov 13, 2021
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.

3 participants