-
Notifications
You must be signed in to change notification settings - Fork 8
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
Implemented a slue of new functions to interact with the realms API #40
Conversation
Copied bedrock realm API functions that also apply to Java over to Java Updated Realm object and BedrockRealmAPI class
Are we wanting to add test cases for all of these calls? |
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.
Thanks for working on this. Left some comments, but yes tests would be good for API calls that are expected to return something from the server, so we know what data we're working with.
These new functions would also need to be added to the doc, with explicit information on what they return.
cc @LucienHH if you have anything to add
Sounds good! Thank you for the feedback and the review. Expect everything to be pushed by the end of today. (in ~4 hours from now max) |
Removed removeAllRealmPlayers function due to invalid request Added params to methods in Realm.js Created tests and responses for the new functions Added documentation for the new functions
For now, I'm not pushing anything related to removing all realm players for now. There seems to be an issue with setting values as null in the body of the requests. I will be creating another PR for this issue to address it better and to keep it separated from here. |
Fixed body of userPermission request Moved functions that are the same into RealmAPI class Renamed removePlayerFromRealm uuid param to xuid
Fixed everything there, there are a couple of small things that still need to be fixed but I already have another PR cooking up to address them. |
Changed configuration parameter to be an Object instead of a String
If you could just make one big list of changes that need to be made it'd be greatly appreciated. It's nice to just have one big push of fixes then the commit history doesn't get all clogged unless we're doing a rebase when merging. |
Don't worry about the PR commits. It will be squashed on merge. |
Updates a Realms configuration. This can be gamerules or general Realm settings | ||
|
||
```js | ||
const configuration = { description:{description: "",name: option:{slotName:"Test",pvp:true,spawnAnimals:true,spawnMonster:true,spawnNPCs:true,spawnProtection:0,commandBlocks:false,forceGameMode:false,gameMode:0,difficulty:2,worldTemplateId:-1,worldTemplateImage:"",adventureMap:false,resourcePackHash:null,incompatibilities:[],versionRef:"",versionLock:false,cheatsAllowed:true,texturePacksRequired:true,timeRequest:null,enabledPacks:{resourcePacks:[""],behaviorPacks:[""]},customGameServerGlobalProperties:null,worldSettings:{sendcommandfeedback:{type:0,value:true}commandblocksenabled:{type:0,value:true},dodaylightcycle:{type:0,value:true},randomtickspeed:{type:1,value:3},naturalregeneration:{type:0,value:true},showtags:{type:0,value:true},commandblockoutput:{type:0,value:true},dofiretick:{type:0,value:false},maxcommandchainlength:{type:1,value:65535},falldamage:{type:0,value:true},tntexplodes:{type:0,value:true},drowningdamage:{type:0,value:true},domobloot:{type:0,value:true},domobspawning:{type:0,value:true},showbordereffect:{type:0,value,:true},showdeathmessages:{type:0,value:true},respawnblocksexplode:{type:0,value:true},doweathercycle:{type:0,value:true},doentitydrops:{type:0,value:true},doimmediaterespawn:{type:0,value:true},freezedamage:{type:0,value:true},pvp:{type:0,value:true},keepinventory:{type:0,value:false},doinsomnia:{type:0,value:true},mobgriefing:{type:0,value:true},dotiledrops:{type:0,value:true},firedamage:{type:0,value:true},functioncommandlimit:{type:1,value:10000},spawnradius:{type:1,value:25},showcoordinates:{type:0,value:true}}}}} |
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.
Maybe out of scope for this PR but providing some sort of config builder would make it a lot easier to interact with the API here. I.e.
const { RealmConfigBuilder } = require('prismarine-realms')
const config = new RealmConfigBuilder()
.setName('')
.setDescription('')
.addWorldSettings(
{ name: 'dofiretick', value: true },
{ name: 'domobloot', value: false}
)
api.changeRealmConfiguration(config)
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.
Yep! I was thinking the same thing. It was really bugging me with the large JSON code block. I'll open another PR relatively soon sometime this week, or I'll just push it along with the next PR I have as a draft right now.
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.
You can remove this function from this PR and add it to a later one if it's not ready
If you have any feedback for anything else that is out of scope or that you think doesn't belong in this PR just let me know and I'll create another PR. Right now I'm just trying to get a good baseline to start from to expand further and improve. |
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.
The purpose of prismarine realms isn't to be an basic API wrapper per se but provide a stable high level lib. If we have to put out a breaking change if the backend changes its data format then it's no good.
So if we either return, store or expose data, it needs to be explicit. If something is supposedly void then generally there should not be any return value in the method.
So I should be making custom types for responses for every function that returns something other than void in index.ts? |
Yes, either don't return anything or doc what is returned |
Updated docs to reflect type changes Formatted what functions return to reflect their custom types
Any other changes that need to be made? |
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 some comments about API. Some of these things should be exceptions. Otherwise if a return value is expected, like status of success, then the example code needs to handle the false cases when calling functions like .open().
|
||
Restores a Realm from a backup | ||
|
||
```js | ||
await api.restoreRealmFromBackup('1234567', '1', '1970-01-01T00:00:00.000Z') | ||
``` | ||
|
||
No output | ||
Either 'Retry again later' or 'true'. Always seems to return 'Retry again later' on the first try. |
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.
Shouldn't this be handled internally with an exception? This doesn't seem like good exposed API.
|
||
### removePlayerFromRealm | ||
|
||
(realmId: string, xuid: string) => Promise\<Realm> |
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.
Why specifically return a Realm here?
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.
In the bedrock Realm API docs, it states it returns all information about the Realm. Should we not be returning all the Realm's information as states in the API docs?
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.
Nevermind, ignore that. I understand now. Thank you
Updates a Realms configuration. This can be gamerules or general Realm settings | ||
|
||
```js | ||
const configuration = { description:{description: "",name: option:{slotName:"Test",pvp:true,spawnAnimals:true,spawnMonster:true,spawnNPCs:true,spawnProtection:0,commandBlocks:false,forceGameMode:false,gameMode:0,difficulty:2,worldTemplateId:-1,worldTemplateImage:"",adventureMap:false,resourcePackHash:null,incompatibilities:[],versionRef:"",versionLock:false,cheatsAllowed:true,texturePacksRequired:true,timeRequest:null,enabledPacks:{resourcePacks:[""],behaviorPacks:[""]},customGameServerGlobalProperties:null,worldSettings:{sendcommandfeedback:{type:0,value:true}commandblocksenabled:{type:0,value:true},dodaylightcycle:{type:0,value:true},randomtickspeed:{type:1,value:3},naturalregeneration:{type:0,value:true},showtags:{type:0,value:true},commandblockoutput:{type:0,value:true},dofiretick:{type:0,value:false},maxcommandchainlength:{type:1,value:65535},falldamage:{type:0,value:true},tntexplodes:{type:0,value:true},drowningdamage:{type:0,value:true},domobloot:{type:0,value:true},domobspawning:{type:0,value:true},showbordereffect:{type:0,value,:true},showdeathmessages:{type:0,value:true},respawnblocksexplode:{type:0,value:true},doweathercycle:{type:0,value:true},doentitydrops:{type:0,value:true},doimmediaterespawn:{type:0,value:true},freezedamage:{type:0,value:true},pvp:{type:0,value:true},keepinventory:{type:0,value:false},doinsomnia:{type:0,value:true},mobgriefing:{type:0,value:true},dotiledrops:{type:0,value:true},firedamage:{type:0,value:true},functioncommandlimit:{type:1,value:10000},spawnradius:{type:1,value:25},showcoordinates:{type:0,value:true}}}}} |
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.
You can remove this function from this PR and add it to a later one if it's not ready
|
||
### opRealmPlayer | ||
|
||
(realmId: string, uuid: string) => Promise\<Realm> |
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.
Why return Realm here?
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.
Same question as here
|
||
### deopRealmPlayer | ||
|
||
(realmId: string, uuid: string) => Promise\<Realm> |
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.
Same comment as above. This does but banPlayerFromRealm doesn't return Realm.
|
||
Opens a Realm. Allows players to join | ||
|
||
```js | ||
await realm.open() | ||
``` | ||
|
||
No output | ||
True if the world has opened |
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.
Can't this be an exception? Otherwise example code needs to handle the false case.
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.
There is no false case, as stated here it doesn't return false, it returns the retry again later string. Should we be returning false from this function considering it's basically false if it says retry again later.
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.
I also understand this too, sorry about that.
So after a second review, some of the doc/types you updated aren't things changed in the code, so instead of making changes there per my previous comments (about how we should throw), what you can do is simply revert those changes (like changes to .open()). We can handle those later, so we can get this merged quicker. Also, @LucienHH do you think the configuration object will change or break in the future? If so we should move that out of the PR until some abstraction is implemented there. If not then I think it's ok if we just leave it as an dynamic object. |
The config can change from update to update so should definitely be abstracted away, funnily enough the config has already changed from the one in this PR. |
What is the status of this PR? what's missing to finish it ? |
@LucienHH @extremeheat what do you think on this PR? |
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.
LGTM, I think previous my comments about API design are not worth blocking otherwise good PR. There are some useful additions here, thanks @IanTapply22 for the work on this
I commented out the |
Thanks for the work on this. We can discuss the remaining changes in the other PR. I'll merge so we can resolve the PR backlog. |
This PR implements a handful of new functions to interact with even more parts of the Minecraft realms API—all the way from banning players to deleting a realm with the specified ID. You can find the added functionality below:
Bedrock & Java
Bedrock
Feel free to leave suggestions or comments if you need anything fixed, added, or updated. Thanks