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

feat(cli): bench pub support split file content #1642

Merged
merged 3 commits into from
May 9, 2024
Merged

feat(cli): bench pub support split file content #1642

merged 3 commits into from
May 9, 2024

Conversation

ysfscream
Copy link
Member

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

Please describe the current behavior and link to a relevant issue.

Issue Number

Example: #123

What is the new behavior?

image image

Please describe the new behavior or provide screenshots.

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

@ysfscream ysfscream added feature This pr is a feature CLI MQTTX CLI labels May 8, 2024
@ysfscream ysfscream added this to the v1.10.0 milestone May 8, 2024
@ysfscream ysfscream requested a review from Red-Asuka May 8, 2024 16:32
@ysfscream ysfscream self-assigned this May 8, 2024
@@ -501,6 +501,7 @@ export class Commander {
'load the parameters from the local configuration file, which supports json and yaml format, default path is ./mqttx-cli-config.json',
)
.option('--file-read <PATH>', 'read the message body from the file', parseFileRead)
.option('--split <CHARACTER>', 'split the input message in a single file by a specified character.')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should set \n as the default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I thought so at first, but I didn't finish.

@@ -114,6 +114,7 @@ declare global {
messageInterval: number
limit: number
verbose: boolean
split: string
Copy link
Member

Choose a reason for hiding this comment

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

This is an optional option, which may be undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

but the limit, verbose, and others are optional, too, right? Was there no impact before?

Copy link
Member

@Red-Asuka Red-Asuka May 9, 2024

Choose a reason for hiding this comment

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

The default value of limit is 0, and verbose is a boolean type option. The default behavior is false, so it has no effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

so we need to set a default value for the split option as well.

Copy link
Member

Choose a reason for hiding this comment

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

It should be similar to the save and config options. If the option is not declared, it will be undefined. If the option is declared but no value is set, it will default to a default value.

@Red-Asuka Red-Asuka merged commit afb2992 into main May 9, 2024
4 checks passed
@Red-Asuka Red-Asuka deleted the ysf/cli branch May 9, 2024 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI MQTTX CLI feature This pr is a feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants