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

Update lua api #39

Merged
merged 5 commits into from
Jun 18, 2024
Merged

Update lua api #39

merged 5 commits into from
Jun 18, 2024

Conversation

robincodex
Copy link
Contributor

  • Add generics to CEntities
  • Fix purchaser type of CreateItem
  • Change return type of CBaseEntity.IsPlayerController and CBaseEntity.IsPlayerPawn

@robincodex
Copy link
Contributor Author

robincodex commented Jun 17, 2024

@Perryvw Hi, Can you merge this pr? I also have a PR on the dota-data

@Perryvw Perryvw merged commit ba4b5e6 into ModDota:master Jun 18, 2024
1 check passed
@Perryvw
Copy link
Member

Perryvw commented Jun 20, 2024

Sorry @robincodex but after discussing this with DG, we decided the generic entity types are more dangerous than just casting, and fundamentally they are the same. With explicit casts you can at least see in your code where you are making some dangerous assumption, with the generic functions like you did, that becomes at lot less clear. You can of course still make the generic versions in your own code as helper functions or maybe even with interface merging.

The IsPlayerController changes are fine so I left those in.

@robincodex
Copy link
Contributor Author

Sorry @robincodex but after discussing this with DG, we decided the generic entity types are more dangerous than just casting, and fundamentally they are the same. With explicit casts you can at least see in your code where you are making some dangerous assumption, with the generic functions like you did, that becomes at lot less clear. You can of course still make the generic versions in your own code as helper functions or maybe even with interface merging.

The IsPlayerController changes are fine so I left those in.

Ok, no problem

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