-
Notifications
You must be signed in to change notification settings - Fork 1.4k
refactor commands/scripts to expose only one named mne #866
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
Conversation
else: | ||
cmd = sys.argv[1] | ||
cmd_path = op.join(mne_bin_dir, 'commands', 'mne_%s.py' % cmd) | ||
cmd = cmd_path + " " + " ".join(sys.argv[2:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might lead to various unpleasant side effects happen there are spaces in the arguments etc. I guess it would be better to use subprocess.Popen or may be runpy would fit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably the easiest would be to use 'subprocess.call([cmdpath] + sys.argv[2:])', not sure if adding ', shell=True' is needed here
note: updated to add cmdpath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a convenience wrapper for subprocess.popen
in utils.py
that could probably be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok now using Popen and hacking the usage message to recommand the "mne browse_raw" syntax.
usage example
|
should be good to go ... |
Please wait and let me try tomorrow. I'll let you know how it goes. |
import mne | ||
|
||
mne_bin_dir = op.dirname(mne.__file__) | ||
valid_commands = sorted(glob.glob(op.join(mne_bin_dir, 'commands', 'mne_*.py'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtering is not needed here, os.listdir will be 50 % faster
The following commands fail on my box (error prevents print help)
|
However these particular scripts are broken themselves, it's not due to the new master command. |
refactor commands/scripts to expose only one named mne
Thanks @agramfort ! I'll open new issues for the points I mentioned. |
as suggested by @yarikoptic