Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

remove positional arguments from CLI #59

Merged
merged 10 commits into from
May 11, 2023
Merged

remove positional arguments from CLI #59

merged 10 commits into from
May 11, 2023

Conversation

dimas1185
Copy link
Contributor

resolves #13

Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

Is it appropriate to some of these options strings into consts for reuse?

e.g. language is used multiple places, should we have a few lines of could like this:

const std::string lang_flags="-l";
const std::string lang_text="Language this object will use."

@ScottBailey
Copy link
Contributor

I feel like most option flags should have both short and long flags. e.g. -n, --name. We could do this in another PR, but it also could be done here. Thoughts?

@ScottBailey ScottBailey self-requested a review April 30, 2023 21:04
@ScottBailey
Copy link
Contributor

Is it appropriate to some of these options strings into consts for reuse?

Per face-to-face discussion, this suggested change's positive impact on maintenance remains up for debate. It is NOT a requirement for this PR.

tools/init.hpp Outdated Show resolved Hide resolved
tools/add_to.hpp Outdated Show resolved Hide resolved
@jolly-fellow
Copy link
Contributor

I feel like most option flags should have both short and long flags. e.g. -n, --name. We could do this in another PR, but it also could be done here. Thoughts?

IMHO it is not necessarily to do for rarely using options. If we dedicate a letter for a rarely using option we close a possibility to use this letter for an option which may be added in the future and will be used more often.

tools/add_to.hpp Outdated Show resolved Hide resolved
tools/init.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

Let's talk about default value for init subcommand.

Some --help examples need to be updated.

Man page has a couple lines that should be deleted.

EDIT: fixed now.

@ScottBailey
Copy link
Contributor

ScottBailey commented May 2, 2023

I feel like most option flags should have both short and long flags. e.g. -n, --name. We could do this in another PR, but it also could be done here. Thoughts?

IMHO it is not necessarily to do for rarely using options. If we dedicate a letter for a rarely using option we close a possibility to use this letter for an option which may be added in the future and will be used more often.

Interesting point! I'm not opposed to this BUT it is significant extra work to remove those now.

EDIT: nothing needs to be done here.

tools/add_to.hpp Outdated Show resolved Hide resolved
tools/add_to.hpp Outdated Show resolved Hide resolved
@ScottBailey
Copy link
Contributor

The following help examples remain incorrect:

  • antler-proj add app --help (antler-proj add app -p ./path-to-project/ -n MyApp -l C++ --comp -O2)

Additionally the following have path and should not:

  • antler-proj remove app --help
  • antler-proj remove dep --help
  • antler-proj remove lib --help

@ScottBailey
Copy link
Contributor

Please consider updating init's path option, current wording is somewhat unclear.

subcommand->add_option("-p", path, "The path to create the project in. Defaults to project name.");

@ScottBailey ScottBailey self-requested a review May 11, 2023 00:19
Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

lgtm

@dimas1185 dimas1185 merged commit 627a7e1 into main May 11, 2023
@dimas1185 dimas1185 deleted the remove-positional-api branch May 11, 2023 00:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Correct command line parser
3 participants