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

add named parameters and correct types for some natives #1204

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

coalaura
Copy link
Contributor

No description provided.

Copy link
Contributor

@JayPaulinCodes JayPaulinCodes left a comment

Choose a reason for hiding this comment

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

Also overall I don't think you need to prefix each parameter with a letter representing the type of the parameter as the types are described in the native documentation anyway.

ENTITY/CreateModelSwap.md Show resolved Hide resolved
@PsychoShock
Copy link
Contributor

PsychoShock commented Oct 11, 2024

Also overall I don't think you need to prefix each parameter with a letter representing the type of the parameter as the types are described in the native documentation anyway.

Yes you need to that. This is call having a signature structure in the code.

Also, instead of spamming the same review comment for everything, please only make one. This is not a good way to review something.

@JayPaulinCodes
Copy link
Contributor

JayPaulinCodes commented Oct 11, 2024

Also overall I don't think you need to prefix each parameter with a letter representing the type of the parameter as the types are described in the native documentation anyway.

Yes you need to that. This is call having a signature structure in the code.

Also, instead of spamming the same review comment for everything, please only make one. This is not a good way to review something.

Apologies regarding the repetition comments, still new to this GitHub mobile app and just now figured out I can I select multiple lines.

As for the prefixing, it doesn't seem to be the standard across other exiting natives.
Also if we were to do a signature structure, would be nice to have it on all the parameters not just some of them in the effected natives for this Pr, unless there is some reasoning I'm not seeing here.

I suppose where I'm getting confused is why is it required, yet not something being enforced across the repo from what I'm seeing right now.
I may be missing something or having a different perception of this so maybe you could help me see where you coming from.

@AvarianKnight
Copy link
Collaborator

There's a mismatch of naming scheme all across the native list, using iSomeType, fSomeType, etc is fine (and probably more useful, considering Lua and JS show as number or integer which don't really get the point across of their types)

@JayPaulinCodes
Copy link
Contributor

There's a mismatch of naming scheme all across the native list, using iSomeType, fSomeType, etc is fine (and probably more useful, considering Lua and JS show as number or integer which don't really get the point across of their types)

Gotcha, thank you for that input.
My comment was coming from a consistency standpoint, but what I'm gathering is that there isn't a set requirement one way or the other so that is good to know.

With that said, considering what you wrote regarding the benefits for Lua and JS, perhaps we could explore the idea of adding a standard for parameter names to the contributor guidelines for PRs moving forward?

@coalaura
Copy link
Contributor Author

I personally don't care either way, prefix or not. I figured for completions sake i'd use the name as its defined, however i do agree that this is not the standard across other native documentations. Let me know and i'll adjust them either way.

Add some parameter description if possible

@JayPaulinCodes i added as much documentation as i was able to provide, figured i would still push it even if its just the parameters being renamed. A bit of progress is better than none was my thought.

@JayPaulinCodes
Copy link
Contributor

JayPaulinCodes commented Oct 11, 2024

@coalaura How did you find the names for the params again?

@coalaura
Copy link
Contributor Author

@coalaura How did you find the names for the params again?

anchient sourcery and witchcraft

@JayPaulinCodes
Copy link
Contributor

@coalaura How did you find the names for the params again?

anchient sourcery and witchcraft

My train of thought was that if we had enough knowledge to find a accurate name for the param, I feel like we would have enough to give the parameter some form of description, but if that isn't the case so be it.

@coalaura
Copy link
Contributor Author

coalaura commented Oct 11, 2024

My train of thought was that if we had enough knowledge to find a accurate name for the param, I feel like we would have enough to give the parameter some form of description, but if that isn't the case so be it.

The issue is more with my limited knowledge of c++. I try to figure out what it does as best as i can in some cases its above my head tho. I will go through one more time and try to add documentation where i can :)

Edit: Went through all the natives i changed one more time and updated the documentation as best as i could.

Copy link
Collaborator

@AvarianKnight AvarianKnight left a comment

Choose a reason for hiding this comment

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

Initial review, only looked at types will review in more detail later.

ENTITY/PlayEntityAnim.md Outdated Show resolved Hide resolved
ENTITY/PlaySynchronizedEntityAnim.md Outdated Show resolved Hide resolved
ENTITY/PlaySynchronizedMapEntityAnim.md Outdated Show resolved Hide resolved
coalaura and others added 5 commits October 28, 2024 23:26
Co-authored-by: Dillon Skaggs <dillskaggs@gmail.com>
Co-authored-by: Dillon Skaggs <dillskaggs@gmail.com>
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.

4 participants