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

[3.2] Enable new CLI11 LeapFormatter for both cleos and leap-util #399

Merged
merged 3 commits into from
Oct 31, 2022

Conversation

vladtr
Copy link
Contributor

@vladtr vladtr commented Oct 27, 2022

In this pr:

  • Updated CLI11 library
  • Enabled new formatter (with lines) for both leap-util and cleos
  • Fixed compile warnings

New updated CLI11 lib will bring lines/pseudographics when invoking --help-all on a command / subcommand:

image

@@ -9420,4 +9420,134 @@ inline std::string Formatter::make_option_usage(const Option *opt) const {




class LeapFormatter : public Formatter {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be pulled out into a separate header for easier integration in the future? Seems like it could be done without having to modify CLI11.hpp at all. Just have cleos and leap-util include it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is autogenerated header during build process of our custom CLI11, which includes other features such as autocomplete/etc, not available in vanilla cli11. It is a separate header in https://github.com/AntelopeIO/CLI11 and is merged into this superheader on build. This header can be moved to leap repo for sure, if you think this would look better, I'll do that (and roll back / close PR in CLI11 with inclusion of this header in build and one more PR there to remove LeapFormatter from repo)

Copy link
Member

Choose a reason for hiding this comment

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

Ignore my comment then. I have not been paying that close attention to this. I'll let @linh2931 review since he has been tracking these changes.

Copy link
Member

Choose a reason for hiding this comment

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

How does that happen? I cloned AntelopeIO/CLI11@bd54546 and the followed the instructions to build it, i.e I did

git checkout bd545461aaca877b07867156037838abc18ad8dc
mkdir build
cd build/
cmake -DCLI11_SINGLE_FILE=ON ..
make -j

But then

grep Leap include/CLI11.hpp

has nothing. (it doesn't generate the same file as CLI11.hpp here in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no, clash of merges out of sync, should be b19d133 one sec, I'll update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

👍 yes now i'm getting something very close to what is in the PR

@vladtr vladtr requested a review from linh2931 October 31, 2022 14:45
Copy link
Member

@linh2931 linh2931 left a comment

Choose a reason for hiding this comment

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

Not this time. For anything involving UIs (cleos/leap-utils), it would be helpful to show the new formats/printouts in the PR description so that reviewers know what changes are made visually.

libraries/cli11/include/cli11/CLI11.hpp Show resolved Hide resolved
for(const App* com: subcommands) {
if(com->get_name().empty()) {
if(!com->get_group().empty()) {
out << make_expanded(com);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this if the name is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to handle "option groups" in CLI11 (in many places there). Not the cleanest way to do it, but I believe due to evolutionary reasons...

}

// Drop blank spaces
tmp = detail::find_and_replace(out.str(), "\n\n", "\n");
Copy link
Member

Choose a reason for hiding this comment

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

Not mach the comment above.

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 will update these comments in CLI11 lib as well to "Drop blank lines", this header is autogenerated


// Drop blank spaces
tmp = detail::find_and_replace(out.str(), "\n\n", "\n");
tmp = tmp.substr(0, tmp.size() - 1);// Remove the final '\n'
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure the end of the string is always '\n'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes in this context. Thank you!

@vladtr
Copy link
Contributor Author

vladtr commented Oct 31, 2022

Not this time. For anything involving UIs (cleos/leap-utils), it would be helpful to show the new formats/printouts in the PR description so that reviewers know what changes are made visually.

Good suggestion, thank you, done - updated description

@vladtr vladtr merged commit 1e95878 into release/3.2 Oct 31, 2022
@vladtr vladtr deleted the update-cli11-lib branch October 31, 2022 20:25
@arhag arhag linked an issue Oct 31, 2022 that may be closed by this pull request
pnx pushed a commit to eosswedenorg/leap that referenced this pull request Nov 22, 2022
…data

Fix unpack data for signing transaction
pnx pushed a commit to eosswedenorg/leap that referenced this pull request Nov 22, 2022
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.

Finalize leap-util output format
4 participants