Skip to content

2446 multiple cli options#3155

Open
ChristosT wants to merge 6 commits into
f3d-app:masterfrom
ChristosT:2446-multiple-cli-options
Open

2446 multiple cli options#3155
ChristosT wants to merge 6 commits into
f3d-app:masterfrom
ChristosT:2446-multiple-cli-options

Conversation

@ChristosT

Copy link
Copy Markdown

Describe your changes

This MR adds support for multivalued options. The new infrastructure is applied on --define and --reset. The parsing of these two flags use no longer custom code. Instead, the two options are tagged with "valueHelper": "<string_list>" in resources/cli-options.json which signifies that they could hold one or more values.

Issue ticket number and link if any

#2446

Checklist for finalizing the PR

  • I have performed a self-review of my code
  • I have added tests for new features and bugfixes
  • I have added documentation for new features
  • If it is a modifying the libf3d API, I have updated bindings
  • If it is a modifying the .github/workflows/versions.json, I have updated docker_timestamp

AI Disclosure

  • I did not use AI to generate any of the content of that pull request
  • I used AI to generate code in that pull request, if yes please disclose which part of the code was generated and with which model.
  • ...

Continuous integration

Please write a comment to run CI, eg: \ci fast.
See here for more info.

@ChristosT
ChristosT requested a review from a team as a code owner May 16, 2026 21:17

@mwestphal mwestphal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Style is not consistent with F3D coding style, but other than that this looks great.

A single design question.

Comment thread application/F3DOptionsTools.h Outdated
Comment thread application/F3DOptionsTools.cxx
@mwestphal

Copy link
Copy Markdown
Member

\ci fast

@ChristosT
ChristosT force-pushed the 2446-multiple-cli-options branch 2 times, most recently from 0c87988 to a2bbccb Compare May 20, 2026 16:47
@mwestphal

Copy link
Copy Markdown
Member

need help moving forward @ChristosT ?

@ChristosT

Copy link
Copy Markdown
Author

I think I understand the failing edge cases. I just need to make some focus time to resolve them. Thank you !

@mwestphal

Copy link
Copy Markdown
Member

I think I understand the failing edge cases. I just need to make some focus time to resolve them. Thank you !

No problem, thanks for the feedback!

@mwestphal

Copy link
Copy Markdown
Member

need help moving forward @ChristosT ?

@ChristosT

Copy link
Copy Markdown
Author

need help moving forward @ChristosT ?

I think it's okay, I just haven't found the time yet... If I cannot complete within a week I can leave it to other members of the community if there is an urgency to complete it.

@mwestphal

Copy link
Copy Markdown
Member

need help moving forward @ChristosT ?

I think it's okay, I just haven't found the time yet... If I cannot complete within a week I can leave it to other members of the community if there is an urgency to complete it.

No urgency, I'm just checking every single PR every two weeks :)

This allows to store either a string or a vector of strings.
The latter becomes useful for options that support multiple values.
fixes warning emited due to [-Werror=unused-variable]
fixes warning emitted due to [-Werror=shadow]
@ChristosT
ChristosT force-pushed the 2446-multiple-cli-options branch from 9708442 to 9ed540a Compare July 12, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for specifying the same CLI app options multiple times

2 participants