Python: Remove one of three filesystem structures #2971
Conversation
This file contains 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
This comment was marked as outdated.
This comment was marked as outdated.
For commands where the entry in python/mrtrix3/commands/ is not a standalone .py file but a directory, previously there were two possible filesystem contents. One was for that sub-directory to contain a .py file with the same name as the command, as determined by the name of the python/commands/ sub-directory, and that file is expected to contain functions called uage() and execute(). The second is for such a file to be absent, but for there to instead exist two files called usage.py and execute.py, which would provide those relevant functions. This commit removes support for the latter.
Lestropie
force-pushed
the
python_remove_third_format
branch
from
5ff0868 to
5e69f18
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Downstream of #2920.
Follows discussion in #2964.
Currently on
dev:cmd/cmdname.cpppython/mrtrix3/commands/cmdname.py.python/mrtrix3/commands/cmdname/cmdname.py, to facilitate inclusion of other files within a sub-directory specific to that command.python/mrtrix3/commands/cmdname/usage.pyandpython/mrtrix3/commands/cmdname/execute.py, which define the relevant entry points.It is being proposed in Allow splitting C++ commands into separate files #2964 that:
cmd/cmdname.cpp.cmd/cmdname/cmdname.cpp, to facilitate inclusion of other files within a sub-directory specific to that command.My proposal here is: especially if #2964 is implemented, providing support for 2.i.b., then it makes sense to remove support for 1.ii.c..
That way the supported filesystem structure per command will be consistent between the two languages. As shown by the content of this PR, there is little to no sacrifice in doing so; conversely there's the benefit of any communication around permissible filesystem structures likely being simpler.