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

Eh added a bit of docs before I make a bigger pr with way more docs #53

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

shmezi
Copy link

@shmezi shmezi commented May 19, 2022

no clue what I did but hopefully this works ?

B ((byte) 0, 1, "Byte"),
/**
* DataType format for Kilo-Bytes
Copy link
Contributor

@lokerhp lokerhp May 19, 2022

Choose a reason for hiding this comment

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

Can’t this be just Kilobytes? (Same for gb / mb)

Copy link
Author

Choose a reason for hiding this comment

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

Could yes, Just wanted to be clear, really just preference-based feel free to change it tho.

Copy link
Owner

Choose a reason for hiding this comment

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

Since this is your PR, you would have to be the one to change it

Copy link
Owner

@mattmalec mattmalec left a comment

Choose a reason for hiding this comment

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

I'm noticing that quite a bit of code was reformatted in a way that breaks a lot of Javadoc comments that I had written out.

I'm trying to think of there's an easier way to fix it besides manually going through all the files.

@shmezi
Copy link
Author

shmezi commented May 19, 2022

Do you use a plugin to format or something, if so maybe I can just add it so it fixes that

Copy link
Owner

@mattmalec mattmalec left a comment

Choose a reason for hiding this comment

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

Overall, I'm pretty impressed you actually went through all these files and was able to document it. There's a few parts that can be picked up and I realize they're kinda nitty, but these are the types of things that can make you stand out when writing docs.

MB((byte) 2, 1048576, "Megabyte"),
/**
* DataType format for Giga-Bytes
Copy link
Owner

Choose a reason for hiding this comment

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

For stuff like this, you don't need to document each individual enum. They're pretty self-explanatory as it is.

STARTING,
/**
* Represents when the server is online
Copy link
Owner

Choose a reason for hiding this comment

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

In this scenario, this makes sense to document the enum. I like what you did here :)

@@ -27,94 +28,130 @@
import java.util.Set;
import java.util.UUID;

/**
* Represents a {@link PteroClient}'s server
Copy link
Owner

Choose a reason for hiding this comment

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

I would check how I did this with the ApplicationServer implementation. I'd ideally like to keep everything using the same conventions if possible.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm also kinda confused why this is all spaced out now.

@@ -22,11 +22,31 @@
import java.time.OffsetDateTime;
import java.util.UUID;

/**
* Represents a sub-user pm a {@link ClientServer}
Copy link
Owner

Choose a reason for hiding this comment

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

Spelling mistake here. Also, like mentioned earlier, I would check how it's documented with User

boolean has2FA();

/**
* Gets the uuid of the {@link ClientSubuser}
Copy link
Owner

Choose a reason for hiding this comment

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

The description and return value are kinda recursive here and explain the same thing. It's important to notate if this method can return null at all.

Suggested change
* Gets the uuid of the {@link ClientSubuser}
* The UUID of the Server.
*
* @return Never-null {@link java.util.UUID} containing the Subuser's UUID.
*/
UUID getUUID();

default String getMemoryFormatted(DataType dataType) {
return String.format("%.2f %s", getMemory() / (dataType.getMbValue() * Math.pow(2, 20)), dataType.name());
}
/**
* Get the current disk space usage
Copy link
Owner

Choose a reason for hiding this comment

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

This stuff is very recursive. We also don't want to start our doc with "Get" if weren't not actually running any logic to actually "get" the value, we're just returning data that's already there.

Suggested change
* Get the current disk space usage
* The raw amount of disk space currently being used by this Server.
* @return The raw amount of disk space, usually in {@link DataType#MB} form

@@ -42,15 +54,37 @@ default String getUptimeFormatted() {
return String.format("%.0fh %.0fm %.0fs", hours, minutes, seconds);
}

/**
* Get the current ram usage
Copy link
Owner

Choose a reason for hiding this comment

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

Stuff like this, same thing as my previous comment on disk space.

@@ -22,6 +22,9 @@
import java.util.List;
import java.util.Optional;

/**
* Represents a Pterodactyl {@link Nest}
Copy link
Owner

Choose a reason for hiding this comment

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

This can be explained better too. I should have some comments around the review saying the same thing.


import java.util.List;
import java.util.Locale;
import java.util.UUID;

/**
* Represents a user on a {@link ApplicationServer} server
Copy link
Owner

Choose a reason for hiding this comment

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

This can be explained better too. Also still confused on why these lines are spread out.

@@ -17,18 +17,29 @@
package com.mattmalec.pterodactyl4j;

/**
* Represents a servers state.
* Represents a server's state.
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks :)

@shmezi
Copy link
Author

shmezi commented May 27, 2022

Ok nice, sorry for any mistakes.. first pr I've made on open source stuff..

@mattmalec
Copy link
Owner

No need to apologize! It's hard to get a PR right the first time on an open source project. My biggest nit is consistency - making sure the docs are consistent and helpful across the entire project. I meant it when I said I was impressed, a couple of things to clean up, but you really did a great job :)

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.

3 participants