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

Fix annotations for docs parser #1338

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

Conversation

DapperDino
Copy link

  • 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)

@sesposito
Copy link
Member

@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.

@DapperDino
Copy link
Author

DapperDino commented Mar 13, 2025

@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 userIDString vs userID. Ideally I would want the annotations to take the shorter naming, or whichever is most readable on a case by case basis, but if the parser was to ignore the actual variable names, then that's how we end up having missing parameters in the docs. This change isn't urgent so there's no rush to get this merged in. I'm happy to make more modifications before we finalize it.

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.

Copy link
Member

@sesposito sesposito left a 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, "")
Copy link
Member

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).

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