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: Increase visibility of printKeysAndValues() method #9089

Closed
wants to merge 6 commits into from

Conversation

warcooft
Copy link
Contributor

@warcooft warcooft commented Jul 30, 2024

Description

This PR makes printKeysAndValues() public so we can use it anywhere.

Why this needs to be changed:

I have a use case where I need to display the contents of the $options property in a command file.
It's easier to reuse the existing printKeysAndValues() than make a new function with similar behavior. This will help us show more details in our commands.

For example: if you type spark optimize -h or just some random stuff like spark optimize asdf, it'll either show you all the possible options or tell you there's a problem and then show you all the options anyway.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added tests needed Pull requests that need tests 4.6 enhancement PRs that improve existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. labels Jul 30, 2024
$expected .= ' [-l] Enable only locator caching.' . PHP_EOL;
$expected .= ' [-d] Disable config and locator caching.' . PHP_EOL;

$this->assertSame($this->getStreamFilterBuffer(), $expected);
Copy link
Member

@kenjis kenjis Jul 31, 2024

Choose a reason for hiding this comment

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

The method outputs the options (format like [-c]) and the values (descriptions).
It seems it is specialized only for command option output.
So the name printKeysAndValues() is too general. I think more specific name is better.
E.g., printOptionsDescription()?

Copy link
Member

@kenjis kenjis Jul 31, 2024

Choose a reason for hiding this comment

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

No. The test code is not correct.
The method is now used in promptByKey(). It is used to display options for user input.
It is not for command option.

Then, printKeysAndValues() is not bad? Any better method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

printOptionsDescription() isn't too bad, but the name is a bit long.

how about printOptionsList() ?

@kenjis kenjis removed the tests needed Pull requests that need tests label Jul 31, 2024
@warcooft
Copy link
Contributor Author

@kenjis so, this PR doesn't need a test?

@kenjis
Copy link
Member

kenjis commented Jul 31, 2024

Yes, it needs test code.
All public methods should be tested unless it is impossible to write test code.
Test code is kind of documentation. It shows how to use the method clearly.

@kenjis
Copy link
Member

kenjis commented Jul 31, 2024

The method seems to be for only promptByKey().
Keys are surrounded by [ ] and are green.
It is not very versatile and maybe should not be made public.

For example, in help messages, we use the following format.
It is not the same as printKeysAndValues() output.

Options:
  -n     Set migration namespace
  -g     Set database group
  --all  Set for all namespaces, will ignore (-n) option

@warcooft
Copy link
Contributor Author

warcooft commented Jul 31, 2024

If so, i think we should add a new one public function to CLI class to achive this purpose. i believe it very useful to improve our command.

Especially when we have a lot of options, implementing --help or -h flags would significantly enhance user experience and efficiency.

@kenjis
Copy link
Member

kenjis commented Jul 31, 2024

--help is already implemented.

$ ./spark optimize --help

CodeIgniter v4.5.4 Command Line Tool - Server Time: 2024-07-31 20:57:11 UTC+00:00

Usage:
  optimize

Description:
  Optimize for production.

@warcooft
Copy link
Contributor Author

warcooft commented Aug 1, 2024

Hha, shame on me. I didn't expect that to exist! 😅

Thanks for letting me know.

@warcooft warcooft closed this Aug 5, 2024
@warcooft warcooft deleted the feat-pkv branch August 5, 2024 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 docs needed Pull requests needing documentation write-ups and/or revisions. enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants