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

Feature:command line options autotomatic help generation #71

Merged
merged 12 commits into from
Aug 19, 2023

Conversation

dragonmux
Copy link
Member

In this PR we extend the substrate/command_line/options subsystem to allow the help strings contained within to then be used to generate automatic help listings for programs.

@dragonmux dragonmux added the enhancement New feature or request label Aug 16, 2023
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #71 (834ea99) into main (542b14a) will decrease coverage by 1.77%.
The diff coverage is 4.68%.

❗ Current head 834ea99 differs from pull request most recent head 8b22eb8. Consider uploading reports for the commit 8b22eb8 to get more accurate results

@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
- Coverage   92.01%   90.25%   -1.77%     
==========================================
  Files          47       47              
  Lines        3208     3273      +65     
  Branches      626      641      +15     
==========================================
+ Hits         2952     2954       +2     
- Misses        186      248      +62     
- Partials       70       71       +1     
Files Changed Coverage Δ
impl/command_line/options.cxx 58.06% <0.00%> (-27.65%) ⬇️
substrate/command_line/arguments 100.00% <ø> (ø)
substrate/command_line/options 83.05% <0.00%> (-6.05%) ⬇️
impl/command_line/arguments.cxx 83.76% <33.33%> (-1.74%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@lethalbit lethalbit left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@amyspark amyspark left a comment

Choose a reason for hiding this comment

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

Before reviewing, I don't see a rationale for this change or how it's intended to be used, so I don't know how to approach it. Mind explaining a bit?

(Also, for future PRs, please do so -- if only through private messaging -- before assigning it to me for review.)

@dragonmux
Copy link
Member Author

dragonmux commented Aug 17, 2023

Apologies Amy, we will fill you in on the complete rationale now via DMs.

The main design goal here though is that having to manually write the help text output for a program and keep it in sync with the option lists is bugprone and difficult, so we wanted to have the framework generate the texts as much as possible automatically, á la Python's argparse, or Rust's Clap frameworks.

@amyspark
Copy link
Collaborator

Ack. Will review this first thing tomorrow together.

Copy link
Collaborator

@amyspark amyspark left a comment

Choose a reason for hiding this comment

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

Hi @dragonmux, there are several nitpicks here and there. I can give a more definite answer if you supply an example or tests that I can feed into Binary Ninja.

impl/command_line/options.cxx Outdated Show resolved Hide resolved
impl/command_line/options.cxx Outdated Show resolved Hide resolved
impl/command_line/options.cxx Show resolved Hide resolved
impl/command_line/options.cxx Outdated Show resolved Hide resolved
impl/command_line/options.cxx Show resolved Hide resolved
impl/command_line/options.cxx Outdated Show resolved Hide resolved
@dragonmux dragonmux force-pushed the feature/command-line-options-auto-help branch from 4da4039 to 807f088 Compare August 19, 2023 13:58
…Set_t::displayHelp()

It is known by the author that the upper-casing of the meta name will break if given non-ASCII characters, but C++ does not provide a good means to solve this for now.
…d a destructor for arguments_t so the compiler doesn't try to auto-gen them and fail
@dragonmux dragonmux force-pushed the feature/command-line-options-auto-help branch from d8bea38 to 834ea99 Compare August 19, 2023 15:25
@dragonmux dragonmux merged commit 8b22eb8 into main Aug 19, 2023
111 of 165 checks passed
@dragonmux dragonmux deleted the feature/command-line-options-auto-help branch August 19, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants