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(cmd): avoid to use CommandAttributes::flags directly #2619

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

PragmaTwice
Copy link
Member

We should use CommandAttributes::GenerateFlags instead of accessing the flags member directly.

@PragmaTwice PragmaTwice requested a review from git-hulk October 23, 2024 17:11
@mapleFU
Copy link
Member

mapleFU commented Oct 23, 2024

Is this to avoid not calling flag_gen fn if having?

@PragmaTwice
Copy link
Member Author

Is this to avoid not calling flag_gen fn if having?

Exactly.

@mapleFU
Copy link
Member

mapleFU commented Oct 23, 2024

What about making it private to avoid some explicit calling? ( Current method lgtm but I think it's easy to use it directly next time)

@PragmaTwice
Copy link
Member Author

What about making it private to avoid some explicit calling? ( Current method lgtm but I think it's easy to use it directly next time)

Done.

@PragmaTwice PragmaTwice requested a review from mapleFU October 24, 2024 05:11
@PragmaTwice
Copy link
Member Author

Also I plan to make key_range and other related members private in the next PR.

@@ -182,9 +228,11 @@ struct CommandAttributes {
// commander object generator
CommanderFactory factory;

uint64_t InitialFlags() const { return flags_; }
Copy link
Member

Choose a reason for hiding this comment

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

Minor question: why InitialFlags() in some cases?

Copy link
Member Author

@PragmaTwice PragmaTwice Oct 24, 2024

Choose a reason for hiding this comment

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

Some times we cannot get the cmd args, so maybe the initial flags is useful. Also I currently don't want to touch some legacy sources.

@PragmaTwice PragmaTwice requested a review from mapleFU October 24, 2024 06:17
template <typename T>
auto MakeCmdAttr(const std::string &name, int arity, const std::string &description, NoKeyInThisCommand no_key,
const AdditionalFlagGen &flag_gen = {}) {
CommandAttributes attr(name, arity, CommandCategory::Unknown, ParseCommandFlags(description, name), flag_gen, no_key,
Copy link
Member

Choose a reason for hiding this comment

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

So NO_KEY has becoming a different ctor fn?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, 0,0,0 is not clear enough.

@PragmaTwice PragmaTwice merged commit c525d7a into apache:unstable Oct 24, 2024
31 checks passed
Copy link

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.

2 participants