Conversation
Some command-line arguments could be one of multiple types, depending on the user's desired input / behaviour. Rather than simply flagging all such instances as "various", thereby placing zero restrictions on inputs and providing minimal information on the interface internally, instead utilise each command-line argument type as a bitwise flag, allowing each command-line argument to nominate what types of input are allowed vs. not. Note that this requires some care internally within the command-line parsing: requirements at the point of command-line parsing can typically only be imposed if the argument type is exclusive. Also fixes some non-functional checks on the CLIs, which neglected to detect expected deviations from previous behaviour. Closes #2580. Supersedes #3067.
This was referenced Mar 11, 2025
Prior to 16dc6b8 these tets were not being properly assessed, which allowed out-of-date copyright headers to persist in the reference test data.
Member
Author
|
daljit46
reviewed
Aug 15, 2025
Lestropie
force-pushed
the
no_type_various
branch
from
90e74e6 to
ee9fb0c
Compare
Lestropie
force-pushed
the
no_type_various
branch
from
ee9fb0c to
90e74e6
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This was referenced Aug 28, 2025
Co-authored-by: Daljit Singh <[email protected]>
Remove user-friendly message if user attempts to provide as input to fixelcfestats a tractogram rather than a pre-computed fixel-fixel connectivity matrix, given that this change in interface is now five years old.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
|
Conflicts: cpp/cmd/fixelconvert.cpp cpp/core/app.h
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
|
Merging with #3116 has broken tests here: "unit tests" are now run in parallel, but some of the tests do involve the writing of filesystem content, and this is resulting in one test deleting the intermediate content from another test. So either:
The guilty parties are:
|
These tests involve filesystem manipulations, and therefore cannot be executed in parallel due to mass erasing of any filesystem content beginning with "tmp*" for all tests.
- Change "format" function argument for many help-page-generating functions from int to bool, as it is only ever evaluated as a bool. - Add default functions to ArgModifierFlags class for clang-tidy.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
daljit46
reviewed
Nov 12, 2025
Member
|
This LGTM. I also agree with renaming tests that aren't independent (unit tests should always be IMO). |
Co-authored-by: Daljit Singh <[email protected]>
Lestropie
added a commit
to Lestropie/dwidenoise2
that referenced
this pull request
Nov 17, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2580.
Supersedes #3067.
More info via 3ce5b54.
Knew that fixing what I think is the wrong implementation via #3067 was going to irritate me; so went ahead and re-implemented in the way I think is preferable. The changes to the CLI tests should show what's different.
For
__print_full_usage__(which remains unused elsewhere to the best of my knowledge), I chose to keep all possible types for a given command-line argument on the same line, withINT/FLOATalways followed by two lower and upper bounds, andCHOICEis always the last one in the list given the risk of one of the possible input strings clashing with one of the argument type identifiers.