Skip to content
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

Supports JEP 330 in 'java' autocompletion #662

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frankslin
Copy link
Contributor

JEP 330 allows execution of single-file source code with "java" command as follows:

$ java HelloWorld.java

This commit updates the bash autocompletion to suggest *.java files in addition to precompiled classes.

JEP 330 allows execution of single-file source code with "java" command as follows:

$ java HelloWorld.java

This commit updates the bash autocompletion to suggest *.java files in addition to precompiled classes.
Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

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

Thanks! This is in principle good, but there are a few things to address:

It causes multiple failures in the test suite because test_2 to test_5 of test/t/test_java.py now start to look for .java files, so we end up with dirs in completions we did not expect. The test suite needs to be fixed to cater this.

The commit message does not follow the format we expect. See CONTRIBUTING.md for why this is important. This needs to be addressed too, but we could do it while (squash-)merging this eventually.

For future reference, be sure to install the pre-commit checks or run them manually otherwise, that would have caught the commit message formatting issue here. Be sure to run the test suite regarding the modifications, too. See CONTRIBUTING.md for more info on both.

@frankslin
Copy link
Contributor Author

Thank you for your review! I was able to repro the test failures.

With the new completion rule looking for individual ".java" files, all the directories are now included in the completion results. I'm puzzled for the right fix. Should I include all the sub-directories in the test? That would make these tests fragile.

Example pytest output (with --vv):

________________________________________________________________________________________ TestJava.test_2 _________________________________________________________________________________________

self = <test_java.TestJava object at 0x7df18f94eac8>
completion = <CompletionResult ['7z/', '_filedir/', '_get_cword/', '_known_hosts_real/', '_longopt/', 'acroread/', 'ant/', 'apt-mar...sha256sum/', 'shared/', 'shells/', 'slackware/', 'ssh/', 'ssh-copy-id/', 'tar/', 'toplevel', 'tox/', 'xrandr/', 'xz/']>
can_list_jar = True

    @pytest.mark.complete("java ")
    def test_2(self, completion, can_list_jar):
        if can_list_jar:
>           assert completion == "b bashcomp.jarred c. toplevel".split()
E           AssertionError: assert <CompletionResult ['7z/', '_filedir/', '_get_cword/', '_known_hosts_real/', '_longopt/', 'acroread/', 'ant/', 'apt-mark/', 'b', 'bashcomp.jarred', 'bsdtar/', 'c.', 'compgen/', 'cvs/', 'dnssec-keygen/', 'dpkg/', 'gdb/', 'htpasswd/', 'info/', 'isql/', 'java/', 'lftp/', 'lilo/', 'make/', 'man/', 'mount/', 'mplayer/', 'mutt/', 'nmap/', 'perl/', 'perldoc/', 'pkgtools/', 'pytest/', 'ri/', 'scp/', 'sftp/', 'sha256sum/', 'shared/', 'shells/', 'slackware/', 'ssh/', 'ssh-copy-id/', 'tar/', 'toplevel', 'tox/', 'xrandr/', 'xz/']> == ['b', 'bashcomp.jarred', 'c.', 'toplevel']
E             +<CompletionResult ['7z/', '_filedir/', '_get_cword/', '_known_hosts_real/', '_longopt/', 'acroread/', 'ant/', 'apt-mark/', 'b', 'bashcomp.jarred', 'bsdtar/', 'c.', 'compgen/', 'cvs/', 'dnssec-keygen/', 'dpkg/', 'gdb/', 'htpasswd/', 'info/', 'isql/', 'java/', 'lftp/', 'lilo/', 'make/', 'man/', 'mount/', 'mplayer/', 'mutt/', 'nmap/', 'perl/', 'perldoc/', 'pkgtools/', 'pytest/', 'ri/', 'scp/', 'sftp/', 'sha256sum/', 'shared/', 'shells/', 'slackware/', 'ssh/', 'ssh-copy-id/', 'tar/', 'toplevel', 'tox/', 'xrandr/', 'xz/']>
E             -['b', 'bashcomp.jarred', 'c.', 'toplevel']
E             Full diff:
E             - ['b', 'bashcomp.jarred', 'c.', 'toplevel']
E             + <CompletionResult ['7z/', '_filedir/', '_get_cword/', '_known_hosts_real/', '_longopt/', 'acroread/', 'ant/', 'apt-mark/', 'b', 'bashcomp.jarred', 'bsdtar/', 'c.', 'compgen/', 'cvs/', 'dnssec-keygen/', 'dpkg/', 'gdb/', 'htpasswd/', 'info/', 'isql/', 'java/', 'lftp/', 'lilo/', 'make/', 'man/', 'mount/', 'mplayer/', 'mutt/', 'nmap/', 'perl/', 'perldoc/', 'pkgtools/', 'pytest/', 'ri/', 'scp/', 'sftp/', 'sha256sum/', 'shared/', 'shells/', 'slackware/', 'ssh/', 'ssh-copy-id/', 'tar/', 'toplevel', 'tox/', 'xrandr/', 'xz/']>

/home/frank/git/bash-completion/bash-completion/test/t/test_java.py:25: AssertionError

@scop
Copy link
Owner

scop commented Dec 27, 2021

I think one good way would be to modify the tests affected like this so that they are run in the fixtures/java dir instead of in fixtures. The cwd option to pytest.mark.complete can be used to accomplish that, see git grep cwd= test/t for examples. That would leave a lot fewer dirs to expect, but a/ would still be there to verify we get the dir in these completions.

Would be a good thing to also add a let's say JEP330.java file (maybe with a link in its contents that points to the JEP) in fixtures/java to make it explicit that we really do want *.java in these completions, as it's probably not that well known feature to many.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants