-
Notifications
You must be signed in to change notification settings - Fork 117
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
Added the Weight Command #34
base: master
Are you sure you want to change the base?
Conversation
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.
Docker won't work with the api key properties at the moment - you'll need to update docker-compose.yml
and Configuration.js
for that.
A few suggestions:
- In order to make adding new commands easier to add, it may be worth putting all api related methods inside their own class and adding it as a property on either the
App
orMinecraft
objects, so to get the data you could just add the lineconst playerData = this.minecraft.apiHandler.getPlayerData(<ign>)
or something similar. - Optional parameters may be helpful too,
!weight [ign] [profile]
where neither parameters returns the data for whoever sent the message on their last saved profile, only ign parameter returning the data for the specified ign on their last saved profile and both arguments returning the specified ign on the specified profile's data.
|
||
async function getApiData(ign) { | ||
delete require.cache[require.resolve('../../../config.json')]; | ||
const config = require('../../../config.json'); |
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.
This will break Docker support as the module will not be found, if you wanted to add to be ability to change the config without restarting the bot you'd have to add a method to the Configuration
class to refresh it every time the API key is used.
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.
Should be fixed in the next commit.
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.
Fixed i think
I don't fully understand this code (with all the this. and classes) but i'll try. |
I tried for quite some time but i can't make it work :/ |
Just added a weight command.