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

CommonClient: add more docstrings and comments #3821

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

qwint
Copy link
Collaborator

@qwint qwint commented Aug 20, 2024

What is this fixing or adding?

adds more documentation to what CommonClient is doing and how you are expected to extend it
all descriptions are based on my understanding of the relevant code so correct me if i'm wrong / if there is better wording

How was this tested?

reading

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Aug 20, 2024
@Exempt-Medic Exempt-Medic added the is: documentation Improvements or additions to documentation. label Aug 20, 2024
Copy link
Contributor

@hesto2 hesto2 left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you 🙏

The Command Processor will parse every method of the class that starts with "_cmd_" as a command to be called
when parsing user input, i.e. _cmd_exit will be called when the user sends the command "/exit".

The decorator @mark_raw can be imported from MultiServer and tells the parser to only split on the first
Copy link
Contributor

Choose a reason for hiding this comment

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

This is super helpful, I didn't know about mark_raw 🔥

Copy link
Contributor

@nicholassaylor nicholassaylor left a comment

Choose a reason for hiding this comment

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

Cannot speak for the correctness of the new docstrings as I am not very familiar with CommonClient code. However, taking a brief look at the code that it documents, nothing seems out of place.

Giving a soft approval as long as someone with more experience with client code looks at this.

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 8, 2024
@NewSoupVi NewSoupVi merged commit 7337309 into ArchipelagoMW:main Sep 26, 2024
17 checks passed
@qwint qwint deleted the cc_docstrings branch September 28, 2024 00:28
AustinSumigray pushed a commit to AustinSumigray/Archipelago that referenced this pull request Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: documentation Improvements or additions to documentation. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants