Skip to content

Conversation

desrosj
Copy link
Member

@desrosj desrosj commented Aug 19, 2025

This splits out the documentation updates from #212 that were unrelated to the overall goals of that pull request.

@desrosj desrosj requested a review from javiercasares August 19, 2025 18:50
@desrosj desrosj self-assigned this Aug 19, 2025
@desrosj desrosj marked this pull request as draft August 19, 2025 19:34
Copy link
Contributor

@kittenkamala kittenkamala left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for doing all this, the clarifying comments etc.

This is the tiniest thing -- Line 154 of readme.md is still a bit awkward. Maybe it could say "This example uses the /home/wptestrunner/ folder..."

On line 156, 166 and 436 are we removing the bash part of ```bash because those commands are linux cli and not bash? (for example bash doesn't support a vim or vi command) -- or is there another reason? Should we put a ```linux cli highlight there instead?

Thanks!

This makes the following improvements:
- Ensure each function, and file have a proper short description.
- Use third-person singular verbs for function summaries.
- Enforces the 80 character limit (120 when indentation is present) for DocBlocks.
- Code should be self-documenting. Avoid explaining every step of the code.
- Multi-line inline comments should start with `/*` not `/**`.

Co-Authored-By: Javier Casares <[email protected]>
@desrosj
Copy link
Member Author

desrosj commented Aug 21, 2025

Thanks! I still have a few more changes I'd like to make.

When I created this PR, I started by copying every documentation change unrelated to adding support for mult-php/multi-commit from the other pull request and my plan was to make any further changes for consistency, and pose some discussion points within the code here.

The README file is still on my to do list.

The reason for removing the syntax highlighting for bash there is not clear. The commit that introduced this change did not really explain. I am leaning towards reverting the removal of those unless a good reason presents itself. There possibly are a code examples where the language could be improved.

@kittenkamala
Copy link
Contributor

I see! I vote we leave them in in that case. Should we make a different PR for the readme?

Re:

Thanks! I still have a few more changes I'd like to make.

When I created this PR, I started by copying every documentation change unrelated to adding support for mult-php/multi-commit from the other pull request and my plan was to make any further changes for consistency, and pose some discussion points within the code here.

The README file is still on my to do list.

The reason for removing the syntax highlighting for bash there is not clear. The commit that introduced this change did not really explain. I am leaning towards reverting the removal of those unless a good reason presents itself. There possibly are a code examples where the language could be improved.

Copy link
Contributor

@kittenkamala kittenkamala left a comment

Choose a reason for hiding this comment

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

I know this is still in progress, but just noticed these:

// Define the array of operations to perform, depending on the SSH connection availability.
// If no SSH connection string is provided, add a local operation to the array.
// If an SSH connection string is provided, add a remote operation to the array.
// When am SSH connection string is not provided, add a local operation to the array.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo:

Suggested change
// When am SSH connection string is not provided, add a local operation to the array.
// When an SSH connection string is not provided, add a local operation to the array.

* Transfer the built WordPress codebase to the remote test environment.
*
* When an SSH connection is configured, rsync is used to copy the files
* required tp rim the WordPress PHPUnit test suite.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Suggested change
* required tp rim the WordPress PHPUnit test suite.
* required to run the WordPress PHPUnit test suite.

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

Projects

Development

Successfully merging this pull request may close these issues.

2 participants