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 cUUID class #3871

Merged
merged 1 commit into from Aug 25, 2017

Conversation

Projects
None yet
3 participants
@peterbell10
Member

peterbell10 commented Jul 25, 2017

Closes #3870

@peterbell10 peterbell10 force-pushed the peterbell10:UUIDs branch 5 times, most recently from 2b8a766 to 52eab11 Jul 25, 2017

/** Sets the player components for the mob heads with player type. */
void SetOwner(const cUUID & a_OwnerUUID, const AString & a_OwnerName,
const AString & a_OwnerTexture, const AString & a_OwnerTextureSignature
); // Exported in ManualBindings.cpp

This comment has been minimized.

@madmaxoft

madmaxoft Jul 30, 2017

Member

When breaking into multiple lines, please break after the first (, too.
(So that all param groups start at the same indent level)

@@ -18,6 +18,7 @@
#include "Protocol/ProtocolRecognizer.h"
#include "CommandOutput.h"
#include "FastRandom.h"
#include "UUID.h"

This comment has been minimized.

@madmaxoft

madmaxoft Jul 30, 2017

Member

I think the include might be unnecessary - the object is only passed through by reference.

This comment has been minimized.

@peterbell10

peterbell10 Jul 30, 2017

Member

You're right. I was think GetUniqueID would be returning a UUID.

@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Jul 30, 2017

How about we only mention the cUUID variants in the Lua API documentation, and then in the cUUID's documentation, we mention that anywhere a cUUID is expected, Cuberite also allows a string representing the UUID.

It will probably need some special handling in CuberitePluginChecker because that tool parses the API docs programmatically, but to me it seems more plugin-author-friendly.

@peterbell10

This comment has been minimized.

Member

peterbell10 commented Jul 30, 2017

I'm slightly hesitant about that because it means all future functions taking UUIDs will have to support strings as well. Not saying that's necessarily bad but it's something to be careful about.

@peterbell10 peterbell10 force-pushed the peterbell10:UUIDs branch from 52eab11 to eb77c9f Jul 30, 2017

@peterbell10

This comment has been minimized.

Member

peterbell10 commented Jul 31, 2017

Is this the only place where I would need to add changes? I'm not really familiar with the plugin checker.

@peterbell10 peterbell10 force-pushed the peterbell10:UUIDs branch from fb1e7cb to 6414c92 Jul 31, 2017

@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Jul 31, 2017

I think it will need more changes than that - it actually provides dummy implementation for all API functions, so that implementation will need to know it can operate on a string, too.

But don't worry about that here, we can fix this later on when we have a plugin that actually uses the stuff.

@@ -8568,7 +8567,7 @@ a_Player:OpenWindow(Window);
Type = "string",
},
},
Notes = "Converts the UUID to a dashed format (\"01234567-8901-2345-6789-012345678901\"). Accepts both dashed or short UUIDs. Logs a warning and returns an empty string if UUID format not recognized.",
Notes = "Converts the UUID to a dashed format (\"01234567-8901-2345-6789-012345678901\"). An alias for UUID:ToLongString()",

This comment has been minimized.

@madmaxoft

madmaxoft Aug 9, 2017

Member

UUID:... -> cUUID:... (multiple occurences)

},
},
Notes = [[
Lexicographically compare bytes with another UUID.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 9, 2017

Member

Compares this UUID with the specified Other UUID, Returns ...
Note that the newlines are not exported into the HTML docs.`

@@ -2476,23 +2630,23 @@ static int tolua_cMojangAPI_GetUUIDsFromPlayerNames(lua_State * L)
lua_newtable(L);

// Get the UUIDs:
AStringVector UUIDs = cRoot::Get()->GetMojangAPI().GetUUIDsFromPlayerNames(PlayerNames, ShouldUseCacheOnly);
std::vector<cUUID> UUIDs = cRoot::Get()->GetMojangAPI().GetUUIDsFromPlayerNames(PlayerNames, ShouldUseCacheOnly);

This comment has been minimized.

@madmaxoft

madmaxoft Aug 9, 2017

Member

Use auto

@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Aug 9, 2017

Huh, for some reason my review on this was still pending. Sorry for the delay, I didn't notice.

{
Desc = [[
Class representing a Universally Unique Identifier.
Functions taking a cUUID will also accept a string convertible to a UUID by FromString.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 17, 2017

Member

This might need some emphasizing. How about:

Note that all Cuberite's API functions that take a cUUID parameter will also accept a string in its place, as long as that string can be converted to a cUUID (using the {{#FromString_1|cUUID:FromString}} function).

(I didn't check the FromString link for validity, please make sure it works :)

@peterbell10 peterbell10 force-pushed the peterbell10:UUIDs branch from ec24e48 to cfa23e9 Aug 17, 2017

@peterbell10

This comment has been minimized.

Member

peterbell10 commented Aug 17, 2017

I've added your suggestion to the docs (tested the link and it worked) as well as rebased onto master.

@bearbin

This comment has been minimized.

Member

bearbin commented Aug 25, 2017

@madmaxoft Merge?

@bearbin bearbin merged commit f4f2fc7 into cuberite:master Aug 25, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@peterbell10 peterbell10 deleted the peterbell10:UUIDs branch Aug 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment