Added Static typings #277
Added Static typings #277
Conversation
…piler hits error if function doesnt exist, so typos are caught with static RPC functions)
| _auth_input_props.push_back(pe) | ||
| for picked_property in input_properties: | ||
| picked_property_entry = _property_cache.get_entry(picked_property) | ||
| _record_input_props.push_back(picked_property_entry) |
There was a problem hiding this comment.
Note that _record_input_props is set here outside authority. Solves #250 and its tested to work locally, as intended in all my other PRs. After all, it is not set anywhere if not here, and before this commit, _record_input_props is the same as _auth_input_props for exclusively the authority player, which makes _record_input_props obsolete to _auth_input_props.
And as mentioned elsewhere, this change is how _auth_state_props and _record_state_props works, so I see this as a bugfix
|
Hey, thank you for the contribution, but unfortunately I cannot accept this PR in its current form. First off, it not only contains static typings, but variable renames, doc adjustments, value changes and some minor refactors even. One PR should ideally do only one thing, as outlined in the contribution section of the README. Second, it's not very pleasant how you just decided to not care about the existing coding convention of the project and just straight up rewrite it without even asking about it 🙂 Since it's a PR it has no negative effects on the project, I'm just sad to see contributor effort go to waste like that. Instead, I recommend creating a discussion about code conventions and static typing either here or on Discord. That way we can decide whether we want a change like this and in what shape or form. Also, if there's parts of this PR that fix other things, please submit them as separate PRs so I can review them. Thanks! |
The only non-static typing in this PR is the handling of
_record_input_propsas its just 1 line and solves #250As for the version bump for each PR, I suggest implementing git hooks which automatically upgrade those values after each PR, because that is the only conflict here (I resolved it, but this has version 1.8.4 just like #276 but I guess if both are merged its good, I will resolve merge conflicts if any)