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

Dimensions Addition #1

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

Conversation

DevMonstropolis
Copy link

I honestly hope I'm doing this correctly. First pull request of stable dimensions.

@Taronyuu
Copy link
Owner

Any specific reason why you decided to rename the Main.java file to StayPut.java?

@DevMonstropolis
Copy link
Author

DevMonstropolis commented Oct 31, 2018 via email

@DevMonstropolis
Copy link
Author

Is something amiss that you would like to review further? I add quite a bit to make the logic for dimensions, and can understand if you need more time.

@Taronyuu Taronyuu closed this Nov 12, 2018
@Taronyuu Taronyuu reopened this Nov 12, 2018
@Taronyuu
Copy link
Owner

Apologies, wrong button =)

Copy link
Owner

@Taronyuu Taronyuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few things you should change in my opinion to make sure it is safe to put on a server.

Also change all the variables to camelCase so they are the same in the whole database. :)

Besides that, looks good! Especially for the first time PR'ing. 😄

return true;
}

protected void sendInfoMessage(CommandSender commandSender){
this.sendMessage(commandSender, "Available commands:");
this.sendMessage(commandSender, "/stayput reload - Reloads the config files");
this.sendMessage(commandSender, "/stayput listdimensions - Lists all the dimensions");
this.sendMessage(commandSender, "/stayput rebuildTables - DO NOT RUN THIS");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe don't add it here if you shouldn't run it? Or add a better message to know why people shouldn't run it.

}

if(executedCommand.equals("rebuildTables")) {
this.sendMessage(commandSender, "-- Deleting and Rebuilding Position --");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a permission check here? I assume not everyone should be allowed to run this command.

public void setYaw(Float yaw) {
this.yaw = yaw;
}

public String getDimension_name() { return dimension_name; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a bit of nitpicking, but could you please format all the methods the same? So always a new line after the { and the } on a new line. Also make sure there is a new line between the methods.

double coordX = position.getCoordinate_x();
double coordY = position.getCoordinate_y();
double coordZ = position.getCoordinate_z();
double coordinate_x = position.getCoordinate_x();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the camelCase format for variables. This is meant for all the variables in your changes.

(And if you find any of mine, please change them to if you want to :))

@Taronyuu
Copy link
Owner

What is the current status of this (and maybe other) PRs?

@DevMonstropolis
Copy link
Author

DevMonstropolis commented Apr 30, 2019 via email

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.

2 participants