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

Improve Example documentation. #610

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

Conversation

firetrqck
Copy link

I felt the urge to make the comments more concise, and in some cases more specific 😅

Copy link

@Picsou993 Picsou993 left a comment

Choose a reason for hiding this comment

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

Original messages are already accurate and concise information. It's also not needed to be restrictive in the comment when the restriction is not something done in the code.

@@ -36,17 +36,17 @@ public static void main(String[] args) {
try {
/*
* We'll be fetching the guild's stats using its ID for this example, but guilds can
* also be looked up using their name, or one of their members' Minecraft UUIDs.
* also be looked up by their name, or one of their member's Minecraft UUIDs.

Choose a reason for hiding this comment

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

Original message is correct.

Copy link
Author

Choose a reason for hiding this comment

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

They have the same meaning but the edit is more grammatically correct and imo roles off the tongue easier.

Choose a reason for hiding this comment

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

The correct way to say it is, I believe, to drop the comma after "name" and use members' instead of member's

We'll be fetching the guild's stats by its ID for this example, but guilds can
also be looked up using their name or one of their members' Minecraft UUIDs.

Copy link
Contributor

Choose a reason for hiding this comment

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

"members" should definitely be plural, since the alternative would be "one of their member". As for "members'" vs "members's", I think it's just a matter of style, but the former is normally preferred (Firefox's spellcheck highlights the latter, if that counts for anything).

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 130 to 131
* This might print out upto 125 seperate usernames, so you may want to comment the following line out if you're
* focusing on other info.

Choose a reason for hiding this comment

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

This 125 members limitation is only an in-game limitation, the endpoint could technically returns more members if there was more players in the guild.

Copy link
Author

@firetrqck firetrqck Aug 8, 2023

Choose a reason for hiding this comment

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

is this better in reflected with cefffd2?

*
* We call `.get()` at the end so that we can use the reply in the same thread.
* The downside is that the current thread freezes (or "blocks") until the API responds.
* The downside is that this is synchronous operation.

Choose a reason for hiding this comment

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

Original message should be kept as we are here losing the fact that the performances are dependant of the API response time.

Copy link
Author

Choose a reason for hiding this comment

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

The current thread being blocked until something happening (in this case the API responding) is the definition of synchronous operation(which dependent the time the API takes to respond is how long the thread takes to be unblocked hence the performance being dependent on api response time)

Copy link
Contributor

Choose a reason for hiding this comment

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

The Hypixel API is very accessible, so it often attracts a lot of new developers, which I catered these examples toward. From a newbie's perspective, I worry that "synchronous" might send them down a rabbit hole that makes their learning curve feel a lot steeper. Words like "blocks" and "thread" are there so that those who understand those concepts get the gist.

125 is a in game limit on how many people can join a guild in hypixel, the endpoint can theoretically return infinite.
@firetrqck firetrqck changed the title Better comment API usage examples Improve Example documentation. Aug 8, 2023
@firetrqck
Copy link
Author

firetrqck commented Aug 8, 2023

I also added a bit to the README..

hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
- An example of getting player information can be found in [GetPlayerExample](https://github.com/HypixelDev/PublicAPI/blob/master/hypixel-api-example/src/main/java/net/hypixel/api/example/GetPlayerExample.java)

Information includes their: UUID, network level(exact), rank, mc version, and more!

Choose a reason for hiding this comment

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

Response data include the player's UUID, network level, rank, client version [<- confirm that], and more!

Copy link
Author

Choose a reason for hiding this comment

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

hypixel-api-example/README.md Outdated Show resolved Hide resolved
hypixel-api-example/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TheNullicorn TheNullicorn left a comment

Choose a reason for hiding this comment

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

Hey! I wrote GetGuildExample, so I left some clarification in the conversations for this PR.

*
* We call `.get()` at the end so that we can use the reply in the same thread.
* The downside is that the current thread freezes (or "blocks") until the API responds.
* The downside is that this is synchronous operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Hypixel API is very accessible, so it often attracts a lot of new developers, which I catered these examples toward. From a newbie's perspective, I worry that "synchronous" might send them down a rabbit hole that makes their learning curve feel a lot steeper. Words like "blocks" and "thread" are there so that those who understand those concepts get the gist.

@@ -36,17 +36,17 @@ public static void main(String[] args) {
try {
/*
* We'll be fetching the guild's stats using its ID for this example, but guilds can
* also be looked up using their name, or one of their members' Minecraft UUIDs.
* also be looked up by their name, or one of their member's Minecraft UUIDs.
Copy link
Contributor

Choose a reason for hiding this comment

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

"members" should definitely be plural, since the alternative would be "one of their member". As for "members'" vs "members's", I think it's just a matter of style, but the former is normally preferred (Firefox's spellcheck highlights the latter, if that counts for anything).

@firetrqck
Copy link
Author

@TheNullicorn as to "members' " vs "members's", I was unaware of the 1st being a convention, and so corrected it and addressed synchronous vs thread blocking in d4f220d, is this language better?

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

Successfully merging this pull request may close these issues.

5 participants