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

Write command output to stdout - Typos/grammar mistakes - JSDoc comments improvements #114

Merged
merged 5 commits into from
Jan 25, 2023

Conversation

over-engineer
Copy link
Contributor

Description

Fixes #113.

The first 4 commits are minor (typos, grammar mistakes, and JSDoc comments). The actual change (related to #113) is in 96ef4ff and affects the following files:

  • lib/cli/index.js
  • lib/cli/commands/wp.js
  • lib/cypress-support/commands.js
  • lib/modules/run.js

It basically introduces the -w (or --write-to-stdout) flag for the wp command:

program
  .command('wp')
  .description('Execute the WordPress CLI in the running container')
  .option('<command>', 'The WP-CLI command to execute')
  .option('-w,--write-to-stdout', 'Whether to write the output to stdout')
  .action((options) => {
    const command = options.args.join(' ');
    wp(command, packageDir, logFile, !!options.writeToStdout);
  });

and adds an additional (boolean) parameter for that flag to:

  • wp() (in lib/cli/commands/wp.js), which simply passes the value to run()

  • run() (in /lib/modules/run.js), which uses that value to determine whether we should write to stdout or not (that way, we prevent CLI users from getting duplicate output)

    if (writeToStdout) {
      process.stdout.write(succeed || stdout);
    }

The new writeToStdout parameter is going to be false, by default. We're only setting the flag for the WP Cypress commands in lib/cli/cypress-support/commands.js.

So, we're essentially changing this:

wp(command) {
  cy.exec(`${wpCypress} wp "${command}"`);
}

to that:

wp(command) {
  cy.exec(`${wpCypress} wp --write-to-stdout "${command}"`);
}

I've tested the changes described in this PR on a couple of projects (and with my own Cypress commands) and AFAICT everything is working as expected.

Let me know if you need any more information! 🙂

Change Log

  • Writes command output to stdout while keeping stderr as it is, to prevent any breaking changes (as described in Feature request - Write command output to stdout #113). It introduces an internally used -w, --write-to-stdout flag for the wp command, to prevent CLI users from getting a duplicate output.
  • In addition to the stdout change, this PR fixes a few minor typos and grammar mistakes
  • and improves JSDoc comments a bit (consistent tags, capitalization, updates mismatching param names, documents object structures, and adds missing @return tags)

Screenshots/Videos

N/A

Types of changes (if applicable):

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist (if applicable):

  • Meets provided linting standards.

- Some param types were starting with a lowercase letter (e.g. `string`), while others were starting with an uppercase letter (e.g. `String`). Switched all of them to lowercase for primitive types. Refer to jsdoc/jsdoc#1046 and https://jsdoc.app/tags-type.html (to see what JSDoc uses) for more information
- Fixed a few typos and minor grammar mistakes
- Fixed a few JSDoc params that were not matching the actual param name
- Some returns were using the `@return` tag, while others were using the `@returns` tag. Switched all of them to `@return` for consistency
- Some `@param`s were ending with a period, while others were not. Switched all of them to end with a period for consistency
- Made sure that all JSDoc comments include a `@return` tag (including `@return {void}` and `@return {Promise<void>}`
- Documented object structures were possible (e.g. `@return {Promise<{code: number, stdout: string, stderr: string}>}`)
- Expanded the documentation of functions like `exec()`, `run()`, etc., where their types could be confusing when you first come in the project
Copy link
Contributor

@PaulREnglish PaulREnglish left a comment

Choose a reason for hiding this comment

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

Tested and reviewed the code. LGTM 🎉

@PaulREnglish PaulREnglish merged commit 82f16eb into bigbite:master Jan 25, 2023
@PaulREnglish PaulREnglish mentioned this pull request Jan 25, 2023
4 tasks
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.

Feature request - Write command output to stdout
2 participants