Skip to content

[Feat] AsyncOfflinePlayerArgument#632

Closed
Bram1903 wants to merge 12 commits intoCommandAPI:dev/devfrom
Bram1903:feat/AsyncOfflinePlayerArgument
Closed

[Feat] AsyncOfflinePlayerArgument#632
Bram1903 wants to merge 12 commits intoCommandAPI:dev/devfrom
Bram1903:feat/AsyncOfflinePlayerArgument

Conversation

@Bram1903
Copy link
Copy Markdown
Contributor

@Bram1903 Bram1903 commented Jan 12, 2025

Description

This pull request introduces the AsyncOfflinePlayerArgument, which extends the SafeOverrideableArgument class. The new class wraps the existing OfflinePlayerArgument within a CompletableFuture<OfflinePlayer>, enabling asynchronous handling of the offline player argument in commands. By using this approach, the logic involving API calls to fetch the offline player will be executed off the main thread, reducing the risk of blocking the server's main thread and improving performance.

PR Checklist

  • Does the pull request have clear and descriptive commit messages that explain the changes made?
  • Does the CommandAPI compile successfully? (This will be verified automatically when the pull request is made.)
  • Has this been tested in a Minecraft server environment, including compatibility with the latest Minecraft version and Spigot or Paper?
  • Are unit tests required for this feature, and if so, have they been added to the pull request?
  • Is documentation included in this pull request, and does it adequately explain the new functionality?

DerEchtePilz
DerEchtePilz requested changes Jan 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@DerEchtePilz DerEchtePilz left a comment

Choose a reason for hiding this comment

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

Generally fine, but I noticed a few minor things that I marked.

Regarding the documentation changes, we are, or rather we have migrated the documentation to a new repository located at https://github.com/CommandAPI/docs
While I consider it fine to accept these for now, you should definitely propose the documentation changes to the docs repository.

@Bram1903
Copy link
Copy Markdown
Contributor Author

Bram1903 commented Jan 12, 2025

Generally fine, but I noticed a few minor things that I marked.

Regarding the documentation changes, we are, or rather we have migrated the documentation to a new repository located at https://github.com/CommandAPI/docs While I consider it fine to accept these for now, you should definitely propose the documentation changes to the docs repository.

Thank you for taking the time to review my code and for your helpful feedback! I’ll make sure to open a PR in the documentation repository to integrate the changes I’ve made here into the new docs repository.

For now, I’ll quickly test everything to confirm that the changes are working as expected, and I’ll keep you updated on the results. Thanks again for the guidance!

DerEchtePilz
DerEchtePilz requested changes Jan 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@DerEchtePilz DerEchtePilz left a comment

Choose a reason for hiding this comment

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

Also, on second thought, because we're planning to continue with the documentation in the docs repo anyway it makes little sense to make changes to the existing documentation here.
So: please propse them to the docs repo and remove the changes to it here.

@Bram1903
Copy link
Copy Markdown
Contributor Author

Bram1903 commented Jan 12, 2025

Tested everything, and reverted the documentation changes.

@Bram1903 Bram1903 requested a review from DerEchtePilz January 12, 2025 21:14
@Bram1903 Bram1903 closed this by deleting the head repository Jan 12, 2025
@Bram1903
Copy link
Copy Markdown
Contributor Author

Bram1903 commented Jan 12, 2025

Yeahh...., so I was forking the docs repo (without all branches selected), so I thought I'll quickly remove the fork and create a new one, but I accidentally removed my CommandAPI (this repo) fork. Gotta love it. LOL

@Bram1903 Bram1903 mentioned this pull request Jan 12, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DerEchtePilz DerEchtePilz Awaiting requested review from DerEchtePilz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Bram1903 @DerEchtePilz