-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix annotations for docs parser #1338
base: master
Are you sure you want to change the base?
Conversation
DapperDino
commented
Mar 13, 2025
- Updated casing/variable names to have annotations match code for the parser.
- Added missing annotations found along the way
- Left some as they were, even though they are causing parser warnings. The parser should be updated to deal with edge cases in the future (for lua and typescript)
@DapperDino My only concern is that there's a lot of places where matching the variable assignment name to the annotation to clear the warning makes it less "readable" or introduces the type in a redundant way (e.g.: fooIn, fooString, etc). I stopped reporting them in the review. I'll leave it to you to decide if they should be changed or not. I understand that matching the implementation is very tedious and error-prone. Maybe we should prioritize improving the annotation parser instead of going down this route, but I'll leave it to the @heroiclabs/devrel team to decide. |
For now I was trying to get them all matching, then the decision can be made whether we want the annotations to be shorter in the cases such as If we then based the variable names in the functions always off the annotations, then that would work, but would that mean the annotations would driving the code too much? I guess not as if we name the variables to be easily readable, it shouldn't be a problem to use that name in the code itself, as it should be named well. |
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.
Left a minor comment, but otherwise 🏆
@@ -1073,7 +1076,7 @@ func (n *RuntimeLuaNakamaModule) httpRequest(l *lua.LState) int { | |||
url := l.CheckString(1) | |||
method := strings.ToUpper(l.CheckString(2)) | |||
headers := l.CheckTable(3) | |||
body := l.OptString(4, "") | |||
content := l.OptString(4, "") |
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.
Perhaps body
is more suitable here? (and for the other runtimes too).