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

Update the search-replace command to follow the VIP-CLI style guide #2079

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

yolih
Copy link
Contributor

@yolih yolih commented Oct 31, 2024

Description

Updated the voice, tense, and format of command descriptions, examples, and usage models for the vip search-replace command to follow the VIP-CLI style guide.

Pull request checklist

New release checklist

Steps to Test

Observe the results of the --help display for the impacted commands:

  • vip
  • vip search-replace

@yolih yolih added the [Type] Documentation Pull request to update documentation. label Oct 31, 2024
@yolih yolih requested a review from terriann October 31, 2024 22:49
Copy link
Contributor

github-actions bot commented Oct 31, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

description: 'Perform Search and Replace explicitly on the provided input <file.sql> file',
usage: 'vip search-replace file.sql --search-replace="from,to" --in-place',
description:
'Perform the search and replace operation and save the results to the local input file "file.sql".',
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention that the S&R operation is performed on file.sql?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having a hard time understanding what needs to be improved in order to address your concern.

Is this wording a bit better?
Perform the search and replace operation on the local input file "file.sql" and overwrite the file with the results.

If not, I welcome a version of the description from you that is clearer. 🙏

Comment on lines -29 to +30
'Search and Replace to the specified output <output.sql> file\n' +
' * Has no effect when the `in-place` flag is used',
'Perform the search and replace operation and save the results to a local clone of the input file named "output-file.sql".',
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear to me (from the previous description) where the output will go. Given the "Has no effect when the in-place flag is used," I would assume that the in-place operation would be performed on the source file (file.sql) and the result will be saved to that very file, not to output-file.sql.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@yolih yolih Nov 4, 2024

Choose a reason for hiding this comment

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

One of the goals of the updated descriptions is to put any limitations or requirements for a command option in the description for the option, not in the usage example.

I agree with you that the "Has no effect" phrase is unclear. What if we updated the description for --output to something like this:

.option(
		'output',
		'Clone the local input file and overwrite the clone with the results of the search and replace operation. Accepts a local file path. Overridden by --in-place if both options are run in the same command.'
	)

(I edited the above description after reading the suggestion from @aswasif007)

Co-authored-by: Volodymyr Kolesnykov <[email protected]>
Copy link

sonarcloud bot commented Nov 4, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Documentation Pull request to update documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants