-
Notifications
You must be signed in to change notification settings - Fork 8
Implemented argcomplete for tab-completion #127
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
Implemented argcomplete for tab-completion #127
Conversation
PicoCentauri
left a comment
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.
Thank you @spyke7!
Looks like a good start. If it just works with one line without further setup even better. I left some firs comments and will try it locally soon.
The docs are the linter are not happy. You can see the errors by clicking on the link in the discussion and also run these locally for better debugging.
The linter should be easily please by running the formatting tox -e format.
For the docs there seemed to be a doubled defined label.
Let me know if you encounter any problems fixing the issues.
src/mdacli/cli.py
Outdated
| argcomplete.autocomplete(ap) | ||
| setup_clients(ap, title=f"{name} Analysis Modules", members=modules) |
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.
Shouldn't this be swapped? After we registered all modules?
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.
I think this will still work as it is just a hook
But I will just swap the lines, and then also it will work.
There is no problem in both I guess
| # Check if it still works if the module name is not the second argument | ||
| subprocess.check_call(["mda", "--debug", args, "-h"]) | ||
|
|
||
| def test_subparser_setup_for_tab_completion(): |
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 is a nice test. But I think we should also have a test that the argcompletion is really setup. Maybe check the argcomplete repo how they do it.
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.
done
|
okk |
|
Ya please check now |
|
I did some minor cleanup. But the completion does not work on my machine. |
|
Wait checking it again @PicoCentauri |
|
@PicoCentauri write the above inside the environment created If inside a virtual env - Here is the video - https://github.com/user-attachments/assets/3f649c75-38ef-41cd-8378-0511dc37e2c0 pip install -e . |
|
Thanks! I will test it again tomorrow! |
PicoCentauri
left a comment
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.
Thanks for the changes. It now also works for me!!!
However, it would be nice if we could avoid having this in the __main__ function. Since this will be used also by downstream projects they now also have to copy this argcomplete logic which will ad some additional boilerplate. Maybe there is a way to avoid having this in the main...
Also, is there a way to do file completion for the -s and -f option only for the files that are supported by MDAnalysis. There are of course fixed choices but want choices only based on the file extension. Not sure if argcomplete can do it.
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.
Since the tests you added didn't really fail even when before the completion was not working we should maybe think about more robust test cases.
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.
The previous one was checking for setup
Setup was perfect before and after also
PYTHON_ARGCOMPLETE_OK was the main thing
It was not related with test cases, but crucial for bash completion
PicoCentauri
left a comment
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.
Very nice. I think we are almost there!
src/mdacli/libcli.py
Outdated
| ) | ||
|
|
||
|
|
||
| def _create_mda_file_completer(format_dict): |
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.
I think you don't need the whole dictionary but just the list of keys. Maybe passing a list of file extension makes this function a bit clearer
| def _create_mda_file_completer(format_dict): | |
| def _create_extension_completer(extension_list): |
We could also make this public as this might be useful in general.
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 please check now, I think previously also, the list was passing?
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.
If you loop through a dictionary what you get are the keys.
PicoCentauri
left a comment
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.
Very nice. I have two last minor comments and we can merge.
| # Determine directory and file prefix | ||
| if os.path.sep in prefix: | ||
| prefix_path = Path(prefix) | ||
| directory = prefix_path.parent if prefix_path.parent != Path() else Path() | ||
| file_prefix = prefix_path.name | ||
| else: | ||
| directory = Path() | ||
| file_prefix = prefix |
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 looks to me like a bit complicated LLM code.
I think one can do it without pulling os in and directly converting the prefix to Path object.
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.
hmmm.
Yeah it's looks a bit complicated,, let me do something about it
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.
removed the os
just used the prefix_path only
previously, I thought that using os will be good, but this also works
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.
I mean if you use pathlib you can do everything with it. I usually try either use the newer object style pathlib exclusively or exclusively stick to os.
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.
yeah indeed
|
Thanks again @spyke7 for the nice work! 🚀 |
|
Thanks @PicoCentauri for guiding me to make a clean PR. And also I will be working on other issue in MDanalysis. |




hi maintainers
@orbeckst @lilyminium @hejamu @joaomcteixeira @PicoCentauri
This PR is related to issue #37
I have implemented the argcomplete library for tab-completion, please check this.
I have updated the cli.py file, and tox.ini, README.rst, pyproject.toml, and also test_cli.py
in the test_cli.py I have added a function
test_subparser_setup_for_tab_completion- it checks the setup for argcomplete.Please check this and if any issue please tell me.
@PicoCentauri please check this
📚 Documentation preview 📚: https://mdacli--127.org.readthedocs.build/en/127/