-
Notifications
You must be signed in to change notification settings - Fork 68
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
Godot NakamaClient.gd issue with "list_matches_async" function #82
Comments
I can confirm the issue, a quick fix is to change the diff --git a/addons/com.heroiclabs.nakama/client/NakamaClient.gd b/addons/com.heroiclabs.nakama/client/NakamaClient.gd
index 2f36498..2859949 100644
--- a/addons/com.heroiclabs.nakama/client/NakamaClient.gd
+++ b/addons/com.heroiclabs.nakama/client/NakamaClient.gd
@@ -590,7 +590,7 @@ func list_leaderboard_records_around_owner_async(p_session : NakamaSession,
# Returns a task which resolves to the match list object.
func list_matches_async(p_session : NakamaSession, p_min : int, p_max : int, p_limit : int, p_authoritative : bool,
p_label : String, p_query : String): # -> NakamaAPI.ApiMatchList:
- return _api_client.list_matches_async(p_session, p_limit, p_authoritative, p_label, p_min, p_max, p_query)
+ return _api_client.list_matches_async(p_session, p_limit, p_authoritative, p_label if p_label else null, p_min, p_max, p_query if p_query else null)
# List notifications for the user with an optional cursor.
# @param p_session - The session of the user. I wonder if a better solution would be to have the auto-generated code always skip encoding empty string. But I fell this might break some other endpoints if there is any that specifically wants an empty string encoded as |
Hey @Faless I'm looking at the .NET client right now to see if we ever encode empty strings. |
@Faless We have the same situation in the .NET client where empty strings get tacked on as query parameters. Unless @zyro happens to know if the server handles a query param with an empty string value equivalently to the absence of the query param. But I'd be surprised if that worked in all cases. |
If filter or query aren't needed the server expects no input for those fields, rather than |
Nice works now, just also dont forget to change the: p_label : String, p_query : String to just p_label,p_query |
Unfortunately, in GDScript, an argument that has a type hint of "String" can't accept null as a value. The only way to allow users to pass null for these arguments to
This scares me because it would make it impossible for a user to send any empty string in a query argument to Nakama, because all empty strings would always be skipped. This would also differ from the behavior of the .NET library, where you can pass in empty strings, it's just that string arguments are nullable in C# (at least before C# 8.0 or when the nullable context is disabled). Even if the current Nakama API has no situation where we'd want to pass an empty string as a query argument (I actually don't know if that's true), if something were added later, the Godot client would be at a disadvantage compared to the .NET library, if we made the decision to always omit empty strings. So, I think @Faless' "quick fix" is actually the best fix. If the choice is between (1) removing the type hint on those arguments entirely, or (2) adding a special case for these two argument on this specific method, I think option nr 2 is the lesser "evil". Option nr 1 would remove type safety which I think would worsen the developer experience. Here's PR #93 implementing that fix |
If i want to list just non-authoritative matches and i set the p_authoritative parameter to false, even though i set label,query to empty string i get: {code:3, message:Label filtering is not supported for non-authoritative matches}, if i set it to true it still wont find be my non-authoritative match.
Thanks beforehand for fixing this issue.
The text was updated successfully, but these errors were encountered: