Dynamic ZSH tab completion for any commands using t.p.usage

[dreid]

ZSH completion is unmaintained and completely unused. It should be removed very very soon.

[dreid]

(In [22831]) Branching to 'zshcomp-remove-3078'

[jknight]

How do you know it's unused? Is it non-functional?

[dreid]

Generated zsh completion files are out of date. The consensus is that they should be regenerated or removed.

[dreid]

(In [22836]) Delete twisted.web2.dav and all references to it except in the autogenerated zsh completion files.

Author: dreid Reviewer: exarkun

Refs #3078 Fixes #3072

[teratorn]

There are various issues here that I want to give thought to... I'm busy today though. Will reply again in a few days.

[radix]

(In [22867]) Delete twisted.web2.dav and all references to it except in the autogenerated zsh completion files.

Author: dreid Reviewer: exarkun

Refs #3078 Fixes #3072

[teratorn]

As others have indicated, I am also not happy with the current state of affairs. Having a static set of completion files that are generated against a particular revision of the source tree is problematic in its current implementation.

There are various options to consider:

1) Remove zsh completion support entirely.

2) Add generation of the completion files as a step in the release procedures.

3) Rewrite the zsh "stub" and the python-side code such that completion options are generated and supplied to zsh dynamically at tab-press time.

I don't have any time right now to do #3, so I would suggest doing #2 as a temporary measure until such time as I can remove the maintenance requirements.

Thoughts?

[exarkun]

Option 2 may be possible, but there are some issues:

• the release procedure needs to keep getting simpler, not more complex so if we go this route, someone needs to write some python code, with unit tests, docstrings, etc, to completely automate the process so that it doesn't make releasing Twisted harder.
• if the files are only generated at release time, then they're potentially wrong for a long time in between releases. This is unfortunate for people using trunk (but that's not a primary concern), but it also means that problems introduced may not be noticed until the release, where they may cause problems for the release.

So one of the other options may be preferable.

[exarkun]

twisted.python.zshcomp is now using deprecated APIs. It's going to break much harder soon if something doesn't happen.

[teratorn]

Thanks for the heads up. I've had the itch to work on this, so I think I'll try and find the time shortly. Will be implementing fully dynamic completion at tab-press time... which should make it easy to support a bash as well.

[dreid, teratorn]

(In [25021]) Branching to 'make-zshcomp-dynamic-3078'

[teratorn]

Alright, I've got something that I'm happy with. It's committed in the branch.

For whomever will be reviewing this, a few points:

• Completions are now generated dynamically at tab-press time, as opposed to the old method of generating static completion function files.
• The old stub file, _twisted_zsh_sub, has been renamed to _twisted and now simply calls out to Twisted and evals the result.
• All the files in twisted/python/zsh are now deprecated.
• The old stub file currently ships with Zsh and references the individual completion functions in twisted/python/zsh, so we need to keep them around for backwards-compatibility. They have been changed to duplicate the functionality of the new stub file.
• I will be submitting the new stub file to the Zsh folks as soon as this branch is approved.
• t.p.usage is modifed to look for a special parameter, "--shell-completion". When it sees this, it calls doShellCompletion() then exits the process.
• --shell-completion takes an argument of the form "<shell name>:<completion position>". The positition is provided by the special zsh parameter, $CURRENT. This is a 1-based index that points to the command-line word that the user has pressed TAB on.
• This same entry point should also be used for Bash support, if anyone ever implements it.
• Test cases provided in twisted/test/test_shellcomp.py

[exarkun]

Not a release blocker for 8.2

[glyph]

Sorry for the long review latency. You should grab some other branches and review them to reduce the number of other things people have to get to :).

1. The all-caps yelling which begins _shellcomp's docstring is unnecessary.
   1. Similarly, ALL_FILES_ARE_DEPRECATED doesn't really serve a purpose; the README is sufficient.
   2. It would be nice if, rather than either of these files, you could have a programmatic warning in the completion files so that zsh packagers would notice these.
   3. Finally, I don't understand zsh too well - do these really need to be packaged this way? Will deleting them really break compatibility with something?
2. I don't like extensionless files. Rather than _twisted, can you call it twisted-completion.zsh or something?
   1. Similarly, README should be README.txt.
3. 2 blank lines between methods, 3 between classes, please.
4. pyflakes errors:

   twisted/python/_shellcomp.py:130: 'sys' imported but unused
   twisted/python/_shellcomp.py:132: 'usage' imported but unused
   twisted/python/_shellcomp.py:178: undefined name 'UsageError'
   twisted/python/_shellcomp.py:188: undefined name 'UsageError'
   twisted/test/test_shellcomp.py:8: redefinition of unused 'os' from line 8
   twisted/test/test_shellcomp.py:8: 'os' imported but unused
   

5. twisted.test is a deprecated place to put stuff; you should be putting stuff into test packages. twisted.python.test.test_shellcomp.
6. the local import in twisted.python.usage (once you've fixed _shellcomp to actually use an imported name so there is a circular dependency) should have a comment next to it explaining its necessity, since such imports are generally frowned upon.
7. I'd recommend opt_shell_completion rather than a hard-coded extra conditional in Options.parseOptions.
8. (I was going to say something about distutils installing the new completion file. Lucky for you, it seems that twisted.python.dist.getDataFiles already deals with this.)
9. twisted.test.test_shellcomp emits several deprecation warnings. The twisted test suite should not emit deprecations. You might want to give the CompatibilityPolicy page a read, and have a look at twisted.trial.unittest.TestCase.flushWarnings.
10. TestOptions and TestOptions2 should have more meaningful names: consider "stub" or "fake" rather than "test" for fixtures like this.
11. ZshTestCase is really two test cases: one which is actually a unit test checking zsh-specific features, the other of which is an integration test examining lots of different code within Twisted. It's bad to confuse these two things. I would suggest spreading the integration test among different packages. i.e. rather than one giant list in ZshTestCase, make a test-case which should be subclassed with some attributes in each individual package, with a list of its scripts. Then the test can act as a compliance test for new things that want to add shell-completion metadata as well. You might want to have a look at #3604 for more discussion of test helpers. Or, for a very limited example of what I'm talking about, twisted.conch.test.test_knownhosts.EntryTestsMixin.
12. There are a number of uncovered lines in _shellcomp. Try running "trial --coverage twisted.test.test_shellcomp". Specifically (enumerated for your fixing convenience):
    1. 167
    2. 177
    3. 178
    4. 188
    5. 197
    6. 417
    7. 475
    8. 476
    9. 489
    10. 610
    11. 611
    12. 650
    13. 651
    14. 669
    15. 670
    16. 677
    17. 678
    18. 679
    19. 680
    20. 681
13. As indicated by the "undefined names" pyflakes errors above, some of these uncovered lines contain somewhat serious bugs.

[teratorn]

Thanks for the thorough review, Glyph. I've been out of the world for a few weeks, so I'm just now seeing this... much other work to do but hopefully I'll get around to this soonish.

[teratorn]

(In [26658]) Branching to 'make-zshcomp-dynamic-3078-2'

[exarkun]

twisted/python/zshcomp.py no longer works in trunk.

[teratorn]

(In [28769]) Branching to 'make-zshcomp-dynamic-3078-3'

[teratorn]

(In [32278]) Branching to 'make-zshcomp-dynamic-3078-4'

[teratorn]

Replying to glyph:

Sorry for the long review latency. You should grab some other branches and review them to reduce the number of other things people have to get to :). 1. The all-caps yelling which begins _shellcomp's docstring is unnecessary. 1. Similarly, ALL_FILES_ARE_DEPRECATED doesn't really serve a purpose; the README is sufficient.

Done

1. It would be nice if, rather than either of these files, you could have a programmatic warning in the completion files so that zsh packagers would notice these.

I'm not sure there is anything I can do here.

1. Finally, I don't understand zsh too well - do these really need to be packaged this way? Will deleting them really break compatibility with something?

Yes, unfortunately, deleting any of the deprecated completion files (_twistd, _tkmktap, etc) WILL horribly break completion for all Twisted commands if the user is using the current stub shipped with zsh:  http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=blob;f=Completion/Unix/Command/_twisted

1. I don't like extensionless files. Rather than _twisted, can you call it twisted-completion.zsh or something?

OK, Done. I believe it will continue to live in the Zsh tree as _twisted as that is their naming convention.

1. Similarly, README should be README.txt.

Done

1. 2 blank lines between methods, 3 between classes, please.

Done

1. pyflakes errors: {{{ twisted/python/_shellcomp.py:130: 'sys' imported but unused twisted/python/_shellcomp.py:132: 'usage' imported but unused twisted/python/_shellcomp.py:178: undefined name 'UsageError' twisted/python/_shellcomp.py:188: undefined name 'UsageError' twisted/test/test_shellcomp.py:8: redefinition of unused 'os' from line 8 twisted/test/test_shellcomp.py:8: 'os' imported but unused }}}

pyflakes-clean now :)

1. twisted.test is a deprecated place to put stuff; you should be putting stuff into test packages. twisted.python.test.test_shellcomp.

Done

1. the local import in twisted.python.usage (once you've fixed _shellcomp to actually use an imported name so there is a circular dependency) should have a comment next to it explaining its necessity, since such imports are generally frowned upon.

I'm not sure what to do here. Are you saying I need some comments here? http://twistedmatrix.com/trac/browser/branches/make-zshcomp-dynamic-3078-4/twisted/python/usage.py#L207

1. I'd recommend opt_shell_completion rather than a hard-coded extra conditional in Options.parseOptions.

I would like to do that, but it is not possible, as we have to remember that usage.Options.parseOptions is being invoked with a most-likely incomplete command-line as it is being edited by the user. So going through the normal machinery will often raise exceptions before opt_shell_comletion will have been invoked.

1. (I was going to say something about distutils installing the new completion file. Lucky for you, it seems that twisted.python.dist.getDataFiles already deals with this.)

OK. I would say something about installing the completion stub file if distutils is running as root, but meh... zsh is already shipping a stub file which we are backwards-compatible with.

1. twisted.test.test_shellcomp emits several deprecation warnings. The twisted test suite should not emit deprecations. You might want to give the CompatibilityPolicy page a read, and have a look at twisted.trial.unittest.TestCase.flushWarnings.

Fixed.

1. TestOptions and TestOptions2 should have more meaningful names: consider "stub" or "fake" rather than "test" for fixtures like this.

Fixed.

1. ZshTestCase is really two test cases: one which is actually a unit test checking zsh-specific features, the other of which is an integration test examining lots of different code within Twisted. It's bad to confuse these two things. I would suggest spreading the integration test among different packages. i.e. rather than one giant list in ZshTestCase, make a test-case which should be subclassed with some attributes in each individual package, with a list of its scripts. Then the test can act as a compliance test for new things that want to add shell-completion metadata as well. You might want to have a look at #3604 for more discussion of test helpers. Or, for a very limited example of what I'm talking about, twisted.conch.test.test_knownhosts.EntryTestsMixin.

OK, I've broken the integration tests out in to the existing test modules which exercised Twisted scripts. If the metaclass is considered too silly/clever/evil I'll get rid of it :)

1. There are a number of uncovered lines in _shellcomp. Try running "trial --coverage twisted.test.test_shellcomp". Specifically (enumerated for your fixing convenience): 1. 167 1. 177 1. 178 1. 188 1. 197 1. 417 1. 475 1. 476 1. 489 1. 610 1. 611 1. 650 1. 651 1. 669 1. 670 1. 677 1. 678 1. 679 1. 680 1. 681

`twisted.python._shellcomp' Now has full line coverage. Hooray!

trial --coverage twisted.python.test.test_shellcomp twisted.scripts.test.test_scripts twisted.conch.test.test_scripts twisted.lore.test.test_scripts

1. As indicated by the "undefined names" pyflakes errors above, some of these uncovered lines contain somewhat serious bugs.

Yep, fixed now.

I've added news files under twisted/topfiles. Not sure if they are also needed under twisted/conch/topfiles and twisted/lore/topfiles (?)

Whew! Will be nice to finally slay this beast.

To manually test tab-completion, I think the only thing you have to do is start zsh and run 'autoload -U compinit; compinit' (normally the first thing in ~/.zshrc). Then with this branch in use (via Combinator or otherwise) you should be able to type e.g. "twistd --reactor=<Tab>" and see results.

A stub file for doing completion for Twisted commands has shipped with zsh for quite a long time now, so you should have it already, without needing to copy twisted/python/twisted-completion.zsh anywhere or fiddle with $fpath.

Ready for review.

[teratorn]

#4084 is dependent on this ticket, so removing 'review' keyword until changes suggested there have been incorporated.

[teratorn]

OK, I've added comments as suggested in #4084. Ready for review.

[exarkun]

Thanks for keeping up with this teratorn. Sorry about the review latency - this is a daunting ticket. :)

1. Documentation
   1. All of the good documentation for the zsh_ attributes usable with usage.Options is hidden in the private twisted.python._shellcomp module. This seems somewhat unfortunate. I would like to see some of this documentation added to usage.Options and some moved into the usage howto. My perspective is that someone trying to use usage.Options is almost certainly going to be looking at the usage.Options API documentation and maybe at the usage howto. If zsh completion isn't covered in one of those, they'll probably never discover it. And without a narrative document, many people probably won't be able to figure out how to use the feature.
   2. If the documentation moves to those discoverable locations, then people probably won't need the "# Tab-completion metadata - see twisted.python._shellcomp" hints currently sprinkled onto every Options subclass in the whole codebase (so they can be dropped - along with the commented out zsh_Something bindings). They can use their existing doc-finding skills to figure out what an attribute on a class means (look at the class docstring).
2. Test coverage
   1. Pretty awesome - very near 100% branch coverage ;) Glyph pointed you at the coverage tool that can't report branch coverage though, so it's not too surprising that there are a couple cases missed. The good tool now is coverage.py (invoked with the --branch option).
   2. In ZshArgumentsGenerator.makeExcludesDict, the case where if len(optList) >= 2: is false is not exercised by the test suite.
   3. In descrFromDoc, the case where elif lines[1] != "" and not lines[1].isspace() is false is not exercised by the test suite.
   4. Beyond those two cases, it would also be nice to see some direct tests for escape. All the branches through it are exercised now - sort of. But there's no test that demonstrates "$" is properly escaped, for example.
   5. What's up with test_genSubcommandList?
   6. in test_zshcomp.py, there's still a TestOptions and TestOptions2 which could use better names.
   7. Also in test_zshcomp.py, there's a usage of flushLoggedErrors that passes self as an argument. That looks like a mistake, albeit one that doesn't cause any practical issues. However, the whole call looks superfluous, since I don't think makeCompFunctionFiles ever logs any errors.
3. Other issues
   1. Since --shell-completion lives in the same namespace as user-defined options, it would be better to try to handle conflicts somehow. The weaker (but easier) option might just be to rename it to something more obscure, perhaps something like --_zsh-shell-completion (though other shells might offer completion features, I bet none of them want the information in exactly the same shape as zsh - otoh, perhaps parameterizing the shell is the way to go, --_shell-completion=zsh, though since we don't actually get to use Options to parse this, simpler is probably better). Still no guarantee conflicts will be avoided, but probably helps. The other option would be to check for a conflict and raise an exception so the developer knows to pick a different name.
   2. twisted.python.zshcomp no longer has a docstring, because the deprecation warning code comes first in the module.
   3. As far as the metaclass/testing complexity goes, I'm willing to give this a try. I don't think we have a good solution to the problem of semi-dynamic test generation. Avoiding tedious code duplication is nice; let's see how this approach holds up. However, it might be nice to have a base class that defines __metaclass__ so that the users can just subclass instead of having to set __metaclass__ themselves.
   4. test-case-name for both twisted/python/_shellcomp.py and twisted/python/zshcomp.py needs updating
   5. doShellCompletion wouldn't suffer from a name that doesn't begin with "do", nor some extra text in the docstring summarizing its purpose/behavior.
   6. Make sure all new classes are new-style
   7. 11.0 appears in a place or two; should be 11.1 now (though hopefully 11.1 will be release pretty soon, so it might end up being 11.2/12.0 in which this change appears depending on timing)
4. Boring coding standard stuff
   1. There's some trailing whitespace here and there which should be cleaned up
   2. Lines should be kept to 80 columns or less where reasonable
   3. Copyright statements should have the year removed from them
   4. Methods should be separated by 2 blank lines. Classes by 3 blank lines.
   5. Test method docstrings shouldn't bother to say "Test that ..." at the beginning.
   6. Avoid the failUnless-style assertion methods, stick to the assertEqual (not assertEquals :/)-style methods.
   7. But self.fail(...) is probably nicer than raise self.failureException(...).
5. Future directions (things to think about and probably file tickets for)
   1. Since zsh completion was introduced, I think usage.Options gained its newer value coercion (really, parsing) feature. It would be nice to see these two features working together more. If an option is declared with a coercer that requires a reactor name, it would be nice to automatically tell zsh what values are valid using that coercer, rather than duplicating the effort. This is nice for efficiency and for reduced code duplication.
   2. The zsh_ naming convention used here doesn't really conform to the Twisted coding standard. The suffix is not dynamically generated - there's just a few different possible values. It would be nice to combine all of these things into a single object and let usage.Options have a single attribute that can refer to such an object.
   3. A potential third option to the --shell-completion conflict issue I mentioned above is to implement this functionality entirely outside of usage.Options. The zsh completion stub could call directly into _shellcomp instead of asking usage.Options to do it. This perhaps suggests doing the easy thing (making a more obscure option name) for now and then removing the option altogether later on.
   4. There's a lot of reaching into the guts of usage.Options from _shellcomp. This suggests usage.Options could use an expanded introspection interface (perhaps this is more than one ticket, each focusing on a particular area of introspection).

It'd also be nice to merge the branch forward to get  a cleaner run on buildbot. Current results don't look too bad, but probably some of that red will go away with a new branch.

I hope this review is comprehensive and next time around we can talk about merging. :) Thanks again.

[teratorn]

(In [32607]) Branching to 'make-zshcomp-dynamic-3078-5'

[teratorn]

Awesome review - I feel much better about the code now, having tackled the issues that have been raised here.

Replying to exarkun:

Thanks for keeping up with this teratorn. Sorry about the review latency - this is a daunting ticket. :) 1. Documentation 1. All of the good documentation for the zsh_ attributes usable with usage.Options is hidden in the private twisted.python._shellcomp module. This seems somewhat unfortunate. I would like to see some of this documentation added to usage.Options and some moved into the usage howto. My perspective is that someone trying to use usage.Options is almost certainly going to be looking at the usage.Options API documentation and maybe at the usage howto. If zsh completion isn't covered in one of those, they'll probably never discover it. And without a narrative document, many people probably won't be able to figure out how to use the feature.

Yep. Good call - I've moved the documentation to the Options docstring, and added a section to the options howto. They reference further docs in the Completions docstring.

1. If the documentation moves to those discoverable locations, then people probably won't need the "# Tab-completion metadata - see twisted.python._shellcomp" hints currently sprinkled onto every Options subclass in the whole codebase (so they can be dropped - along with the commented out zsh_Something bindings). They can use their existing doc-finding skills to figure out what an attribute on a class means (look at the class docstring).

Yep, done.

1. Test coverage 1. Pretty awesome - very near 100% branch coverage ;) Glyph pointed you at the coverage tool that can't report branch coverage though, so it's not too surprising that there are a couple cases missed. The good tool now is coverage.py (invoked with the --branch option). 1. In ZshArgumentsGenerator.makeExcludesDict, the case where if len(optList) >= 2: is false is not exercised by the test suite. 1. In descrFromDoc, the case where elif lines[1] != "" and not lines[1].isspace() is false is not exercised by the test suite. 1. Beyond those two cases, it would also be nice to see some direct tests for escape. All the branches through it are exercised now - sort of. But there's no test that demonstrates "$" is properly escaped, for example.

OK I think tests are in order now. All the branches should be covered and I've added direct tests for escape(). The actual tests in test_shellcomp.py might not *quite* cover all the branches - but if you also run tests which use ZshScriptTestMixin then you should see them all covered.

1. What's up with test_genSubcommandList?

Gone now.

1. in test_zshcomp.py, there's still a TestOptions and TestOptions2 which could use better names.

Done.

1. Also in test_zshcomp.py, there's a usage of flushLoggedErrors that passes self as an argument. That looks like a mistake, albeit one that doesn't cause any practical issues. However, the whole call looks superfluous, since I don't think makeCompFunctionFiles ever logs any errors.

Fixed.

1. Other issues 1. Since --shell-completion lives in the same namespace as user-defined options, it would be better to try to handle conflicts somehow. The weaker (but easier) option might just be to rename it to something more obscure, perhaps something like --_zsh-shell-completion (though other shells might offer completion features, I bet none of them want the information in exactly the same shape as zsh - otoh, perhaps parameterizing the shell is the way to go, --_shell-completion=zsh, though since we don't actually get to use Options to parse this, simpler is probably better). Still no guarantee conflicts will be avoided, but probably helps. The other option would be to check for a conflict and raise an exception so the developer knows to pick a different name.

Renamed to --_shell-completion. I think this is sufficient to avoid accidental conflicts. The shell type, and the index of the command-line word being completed are already parameterized. The only acceptable form is

• some_command --foo --bar --_shell-completion $SHELL:$CURRENT

They must be the last two words on the command-line. $SHELL will be the string "zsh" in the case of zsh, and $CURRENT is a one-based index that points to the command-line word being completed... We don't make /much/ use of this index currently, but it's possible we might make more use of it in the future, and for other shells who knows, so I don't think it hurts to make it part of the protocol.

1. twisted.python.zshcomp no longer has a docstring, because the deprecation warning code comes first in the module.

Fixed.

1. As far as the metaclass/testing complexity goes, I'm willing to give this a try. I don't think we have a good solution to the problem of semi-dynamic test generation. Avoiding tedious code duplication is nice; let's see how this approach holds up. However, it might be nice to have a base class that defines __metaclass__ so that the users can just subclass instead of having to set __metaclass__ themselves.

OK.

1. test-case-name for both twisted/python/_shellcomp.py and twisted/python/zshcomp.py needs updating

Done.

1. doShellCompletion wouldn't suffer from a name that doesn't begin with "do", nor some extra text in the docstring summarizing its purpose/behavior.

Done.

1. Make sure all new classes are new-style

Done.

1. 11.0 appears in a place or two; should be 11.1 now (though hopefully 11.1 will be release pretty soon, so it might end up being 11.2/12.0 in which this change appears depending on timing)

Updated to 11.1

1. Boring coding standard stuff 1. There's some trailing whitespace here and there which should be cleaned up

Fixed.

1. Lines should be kept to 80 columns or less where reasonable

Fixed.

1. Copyright statements should have the year removed from them

Done.

1. Methods should be separated by 2 blank lines. Classes by 3 blank lines.

Done.

1. Test method docstrings shouldn't bother to say "Test that ..." at the beginning.

Done.

1. Avoid the failUnless-style assertion methods, stick to the assertEqual (not assertEquals :/)-style methods.

Done.

1. But self.fail(...) is probably nicer than raise self.failureException(...).

Fixed.

1. Future directions (things to think about and probably file tickets for) 1. Since zsh completion was introduced, I think usage.Options gained its newer value coercion (really, parsing) feature. It would be nice to see these two features working together more. If an option is declared with a coercer that requires a reactor name, it would be nice to automatically tell zsh what values are valid using that coercer, rather than duplicating the effort. This is nice for efficiency and for reduced code duplication.

Haven't put any thought in to this yet, but yeah, it's worth considering.

1. The zsh_ naming convention used here doesn't really conform to the Twisted coding standard. The suffix is not dynamically generated - there's just a few different possible values. It would be nice to combine all of these things into a single object and let usage.Options have a single attribute that can refer to such an object.

Yep, good idea. This is implemented now and documented. A single attribute, shellComp, replaces all the zsh_* mess. shellComp should be an instance of usage.Completions, which provides an interface for specifying completion metadata in a fully generalized way that can supply zsh tab-completion and other shells/UIs.

1. A potential third option to the --shell-completion conflict issue I mentioned above is to implement this functionality entirely outside of usage.Options. The zsh completion stub could call directly into _shellcomp instead of asking usage.Options to do it. This perhaps suggests doing the easy thing (making a more obscure option name) for now and then removing the option altogether later on.

I see a couple issues with this idea, and I think the problem is sufficiently solved by renaming --shell-completion, so I'll just punt here.

1. There's a lot of reaching into the guts of usage.Options from _shellcomp. This suggests usage.Options could use an expanded introspection interface (perhaps this is more

than one ticket, each focusing on a particular area of introspection).

Yeah, will have to think about this and file tickets as appropriate.

It'd also be nice to merge the branch forward to get  a cleaner run on buildbot. Current results don't look too bad, but probably some of that red will go away with a new branch.

Merged forward. And all Twisted tests passing on my local machine, but I suppose I should fire off some slave builds.

I hope this review is comprehensive and next time around we can talk about merging. :) Thanks again.

Awesome. Ready for review.

[teratorn]

(In [32715]) parameterize command name and improve completion stub to recognize valid output instead of blindly eval'ing We now output a properyly formed completion function including the "#compdef $COMMAND" line at the top, where before we just outputted the body of a completion function. Including the #compdef line lets us recognize valid output in the stub file, and it's just treated as a comment line when eval'ing. And it helps in debugging, as the output may be redirected to a file and used with zsh as a stand-alone completion function. Refs: #3078

[teratorn]

(In [32716] Update deprecated completion files to newest code)

[teratorn]

I added a couple final tweaks. Ready for review.

[teratorn]

(In [32725]) restore new completion stub - somehow I accidentally replaced it with the ancient one Refs: #3078

[teratorn]

 build results - some syntax errors on older Pythons still need fixing. Working on it..

[teratorn]

(In [32726]) correct epytext formatting errors Refs: #3078

[teratorn]

(In [32729] fix syntax errors on old crummy Pythons)

OK I think I've fixed up all the errors identified by buildbot. Ready for review now this time for sure.

[exarkun]

Thanks! This review looks long, but a lot of it is simple coding standard stuff and should be trivial to address.

1. doc/core/howto/options.xhtml
   1. the paragraph starting Edit the new file would benefit from an associated example.
   2. you can make a link to the API documentation for twisted.python.usage.Completions by writing <a class="API" base="twisted.python.usage">Completions</a> instead of using a code tag.
2. twisted/python/usage.py
   1. Some direct unit tests for the new CompleteXXX classes would be nice.
   2. Some class docstrings use @param when they mean @ivar
   3. Considering only zsh is supported now, are all the checks for shell type needed? They seem to largely be in private methods, which we can easily change when we want to support a new shell type. At minimum I'd like ZSH and BASH to be private.
   4. CompleteList and CompleteMultiList seem like they might be easier to work with if they took a single list instead of *args.
3. twisted/python/_shellcomp.py
   1. In the module docstring, if you use L{} instead of C{} you'll get API documentation links.
   2. Similarly in other docstrings in this module.
   3. Typo in the first comment in shellComplete - currenlty
   4. Missing some whitespace around the - operator on line 81
   5. Avoid using hasattr - it masks exceptions. Use getattr(obj, name, None) is not None (or substitute another sentinel if None is a valid value for the attribute)
   6. By convention, if a class has some attributes and has an __init__ that accepts parameters with the same name as those attributes (or even just very similar names), then we document it all using @ivar markup in the class docstring and leave the __init__ docstring absent.
   7. ZshArgumentsGenerator also has a bunch of undocumented attributes.
   8. None of optParams_d, optFlags_d, or optAll_d conforms to the naming convention. I'd suggest something more along the lines of (eg) paramNameToDefinition, flagNameToDefinition, allOptionsNameToDefinition.
   9. ZshArgumentsGenerator.writeExtras will raise ValueError sometimes, it should document this.
   10. When some epytext markup text extends beyond one line, it's best formatted by indenting subsequent lines by 4 spaces. Some of the docstrings in this code indents it to align with the beginning of the first line; some doesn't indent it at all. At a minimum it's nice for this to at least have local consistency.
   11. Two commented out calls to tmp.extend in makeExcludesDict can probably be deleted
   12. Lots of names with underscores in them in writeOpt, eg descr_field, multi_field, etc. Please rename.
   13. Typo in comment in getDescription - lets try to get it, should be let's
   14. Also in getDescription, if obj: should be if obj is not None:, since the comparison is about whether the attribute was found on self.options, not about the truth value of the attribute value.
4. twisted/python/test/test_zshcomp.py
   1. There's still some usage of trial's fail* methods.
   2. And some usage of assertEquals instead of assertEqual
5. twisted/python/test/test_shellcomp.py
   1. use twisted.python.reflect.namedAny instead of __import__ and getattr
   2. Still a stray flushWarnings call sitting at the end of test_genZshFunction
   3. More assertEquals here too
   4. Three cases in test_incompleteCommandLine? Sounds like three test methods. :)
   5. Docstrings should be formatted with the triple quotes on their own lines
   6. Too much whitespace in the dict definitions in SimpleProgOptions
6. Elsewhere
   1. Thanks for those new docs!
   2. Too much whitespace in the compData definitions on other Options subclasses throughout the codebase.
   3. in twisted/scripts/mktap.py, there's docs and completion data for the type option. It suggests that xml, source, and pickle are supported. I think that xml is no longer supported. Please file a new ticket for removing the references to the xml type.
   4. It seems like tkconch.py and conch.py would benefit from sharing some code. File a separate refactoring ticket if you don't want to handle it here.
   5. Killer feature for automatic completion generation would be to have trial complete fully-qualified Python names, not just .py filenames.
   6. The 3078.bugfix file has odd contents. Consider that this is documentation intended for people looking at a new Twisted release, trying to figure out what's different.

Thanks again! I think this ticket is getting really close.

[teratorn]

Replying to exarkun:

Thanks! This review looks long, but a lot of it is simple coding standard stuff and should be trivial to address.

Very good.

1. doc/core/howto/options.xhtml 1. the paragraph starting Edit the new file would benefit from an associated example.

Done.

1. you can make a link to the API documentation for twisted.python.usage.Completions by writing <a class="API" base="twisted.python.usage">Completions</a> instead of using a code tag.

Done.

1. twisted/python/usage.py 1. Some direct unit tests for the new CompleteXXX classes would be nice.

Added now.

1. Some class docstrings use @param when they mean @ivar

OK - I think I've gone through and used @ivar where appropriate.

1. Considering only zsh is supported now, are all the checks for shell type needed? They seem to largely be in private methods, which we can easily change when we want to support a new shell type. At minimum I'd like ZSH and BASH to be private.

I've made it _ZSH and _BASH now - I don't think the explicit check for _ZSH is too bad - and it lets us keep the tests which verify that NotImplementedError is raised if a bad constant is used.

1. CompleteList and CompleteMultiList seem like they might be easier to work with if they took a single list instead of *args.

I suppose so - done now.

1. twisted/python/_shellcomp.py 1. In the module docstring, if you use L{} instead of C{} you'll get API documentation links.

Done - thanks.

1. Similarly in other docstrings in this module.

Done.

1. Typo in the first comment in shellComplete - currenlty

Fixed.

1. Missing some whitespace around the - operator on line 81

Fixed.

1. Avoid using hasattr - it masks exceptions. Use getattr(obj, name, None) is not None (or substitute another sentinel if None is a valid value for the attribute)

Done.

1. By convention, if a class has some attributes and has an __init__ that accepts parameters with the same name as those attributes (or even just very similar names), then we document it all using @ivar markup in the class docstring and leave the __init__ docstring absent.

OK - I think I've made this change everywhere appropriate.

1. ZshArgumentsGenerator also has a bunch of undocumented attributes.

Documented now.

1. None of optParams_d, optFlags_d, or optAll_d conforms to the naming convention. I'd suggest something more along the lines of (eg) paramNameToDefinition, flagNameToDefinition, allOptionsNameToDefinition.

Yeah good call - renamed as suggested.

1. ZshArgumentsGenerator.writeExtras will raise ValueError sometimes, it should document this.

Done.

1. When some epytext markup text extends beyond one line, it's best formatted by indenting subsequent lines by 4 spaces. Some of the docstrings in this code indents it to align with the beginning of the first line; some doesn't indent it at all. At a minimum it's nice for this to at least have local consistency.

OK - I've changed everything to a 4-space indent.

1. Two commented out calls to tmp.extend in makeExcludesDict can probably be deleted

Removed.

1. Lots of names with underscores in them in writeOpt, eg descr_field, multi_field, etc. Please rename.

Fixed.

1. Typo in comment in getDescription - lets try to get it, should be let's

Fixed.

1. Also in getDescription, if obj: should be if obj is not None:, since the comparison is about whether the attribute was found on self.options, not about the truth value of the attribute value.

Fixed.

1. twisted/python/test/test_zshcomp.py 1. There's still some usage of trial's fail* methods.

Fixed - since zshcomp.py is merely being deprecated, I didn't think doing any extra work on it was appropriate, but I guess it doesn't hurt to make these basic cleanups.

1. And some usage of assertEquals instead of assertEqual

Fixed.

1. twisted/python/test/test_shellcomp.py 1. use twisted.python.reflect.namedAny instead of __import__ and getattr

Done.

1. Still a stray flushWarnings call sitting at the end of test_genZshFunction

I've added a comment explaining the reason for this call - it's necessary to prevent DeprecationWarnings from being dumped all over the screen of the person running tests.

1. More assertEquals here too

Fixed.

1. Three cases in test_incompleteCommandLine? Sounds like three test methods. :)

Done.

1. Docstrings should be formatted with the triple quotes on their own lines

Fixed.

1. Too much whitespace in the dict definitions in SimpleProgOptions

I've cleaned these up to my liking.

1. Elsewhere 1. Thanks for those new docs!

Sure.

1. Too much whitespace in the compData definitions on other Options subclasses throughout the codebase.

I've gone through and removed whitespace where I could, and as I thought appropriate. I'm not exactly sure what "too much whitespace" means.

1. in twisted/scripts/mktap.py, there's docs and completion data for the type option. It suggests that xml, source, and pickle are supported. I think that xml is no longer supported. Please file a new ticket for removing the references to the xml type.

Done. #5294

1. It seems like tkconch.py and conch.py would benefit from sharing some code. File a separate refactoring ticket if you don't want to handle it here.

Done. #5295

1. Killer feature for automatic completion generation would be to have trial complete fully-qualified Python names, not just .py filenames.

Would be cool, but I'm not sure how to do it :)

1. The 3078.bugfix file has odd contents. Consider that this is documentation intended for people looking at a new Twisted release, trying to figure out what's different.

OK - I've changed the wording to hopefully better reflect the "bugfix" nature of the changes.

Thanks again! I think this ticket is getting really close.

Thanks for the detailed review - I'll be quite happy when this finally lands.

Branch build looks clean:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/make-zshcomp-dynamic-3078-5

[exarkun]

1. Don't use super in the Completer subclasses (CompleteFiles, at least, uses it). super is too hard to use correctly (and this use is incorrect, since Completer does not use it).
2. Also, Completer doesn't seem to the natural top of this inheritance hierarchy. It compiles arbitrary filenames. I'd expect the top of the hierarchy to have no particular completion logic, and CompleteFiles to be the thing in charge of files.
3. "too much whitespace" means that instead of

       compData = usage.Completions(
                      optActions={"logfile"     : usage.CompleteFiles("*.log"),
                                  "certificate" : usage.CompleteFiles("*.pem"),
                                  "privkey"     : usage.CompleteFiles("*.pem")}
                      )
   

   please write

       compData = usage.Completions(
                      optActions={"logfile": usage.CompleteFiles("*.log"),
                                  "certificate": usage.CompleteFiles("*.pem"),
                                  "privkey": usage.CompleteFiles("*.pem")}
                      )
   

   Sorry for not making that clear.
4. in _twistd_unix.py, I don't think prefix should use Completer
5. ZshCompleterTestCase looks good, but the test method docstrings still need some work.
6. There's a trivial conflict to resolve in twisted/scripts/test/test_scripts.py

Thanks!

[teratorn]

(In [32836]) address review comments

• don't use super()
• remove CompleteNothing and have Completer take its place
• CompleteFiles defaults to all files/dirs (*)
• --prefix option doesn't complete filenames anymore
• improve docstrings on ZshCompleterTestCase docstrings

Refs: #3078

[teratorn]

Forgot to use Refs: - (In [32833] remove extra whitespace)

[teratorn]

(In [32839]) Branching to 'make-zshcomp-dynamic-3078-6'

[teratorn]

(In [32841]) Merge forward Refs: #3078

[teratorn]

OK thanks - I think I've addressed all the review points.

Branch build:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/make-zshcomp-dynamic-3078-6

Ready for review.

[exarkun]

1. There are some pydoctor errors:

   twisted.python._shellcomp.ZshArgumentsGenerator: invalid ref to twisted.usage.Completer
   twisted.python._shellcomp.ZshArgumentsGenerator: invalid ref to twisted.usage.Options
   twisted.python._shellcomp.ZshBuilder.write: invalid ref to twisted.usage.Options
   twisted.python._shellcomp.ZshBuilder: invalid ref to twisted.usage.Options
   twisted.python._shellcomp.ZshSubcommandBuilder: invalid ref to twisted.usage.Options
   

2. We're going to delete mktap shortly. Do we need to do anything to zsh completion support to account for this?
3. In ZshCompleterTestCase test methods, change docstrings from "Test X() class" to "X". Also, try not to write "correct". Of course we want correct behavior; what the docstring should convey is what the correct behavior is so a future maintainer can understand the test.
4. Someday I think it would be good to avoid duplicating the default value in the compData definition. Can you file a ticket for automatically extracting that from the option definition and re-using it in the completion output?
5. I'm a little bit uncertain about the references to _shellcomp in both the news fragment and the twisted.python.zshcomp deprecation warning. Users aren't allowed to use this module, so I'm not sure we should even tell them about it. But I don't know why anyone would have been using twisted.python.zshcomp directly anyway - the only use intended was via zsh_ attributes on an Options class. So, I dunno.

I made some formatting changes - r32898, r32900, r32901, r32902, r32903. The rest looks good. Please merge when you're satisfied regarding all of the above review points.

[teratorn]

Replying to exarkun:

1. There are some pydoctor errors: {{{ twisted.python._shellcomp.ZshArgumentsGenerator: invalid ref to twisted.usage.Completer twisted.python._shellcomp.ZshArgumentsGenerator: invalid ref to twisted.usage.Options twisted.python._shellcomp.ZshBuilder.write: invalid ref to twisted.usage.Options twisted.python._shellcomp.ZshBuilder: invalid ref to twisted.usage.Options twisted.python._shellcomp.ZshSubcommandBuilder: invalid ref to twisted.usage.Options }}}

Fixed, thanks.

1. We're going to delete mktap shortly. Do we need to do anything to zsh completion support to account for this?

I've commented on that ticket.

1. In ZshCompleterTestCase test methods, change docstrings from "Test X() class" to "X". Also, try not to write "correct". Of course we want correct behavior; what the docstring should convey is what the correct behavior is so a future maintainer can understand the test.

OK I think it's a lot better now.

1. Someday I think it would be good to avoid duplicating the default value in the compData definition. Can you file a ticket for automatically extracting that from the option definition and re-using it in the completion output?

I don't really know what you mean here - it already gets all the default values out of the optFlags and optParameters lists - you only define things in compData if you want to override a default, or provide additional metadata to improve the completion experience.

1. I'm a little bit uncertain about the references to _shellcomp in both the news fragment and the twisted.python.zshcomp deprecation warning. Users aren't allowed to use this module, so I'm not sure we should even tell them about it. But I don't know why anyone would have been using twisted.python.zshcomp directly anyway - the only use intended was via zsh_ attributes on an Options class. So, I dunno.

Yeah good catch. I've removed the references to _shellcomp and instead refer to t.p.usage since that now contains the publicly exposed tab-completion system usable by third parties.

I made some formatting changes - r32898, r32900, r32901, r32902, r32903. The rest looks good. Please merge when you're satisfied regarding all of the above review points.

Looks good - I totally missed the fact that you suggested to remove the space between the key and : charater in dictionaries.

[teratorn]

(In [32921]) Merge make-zshcomp-dynamic-3078-6: New tab-completion system Author: teratorn Reviewer: glyph, exarkun Fixes: #3078

Deprecates t.p.zshcomp in favor of a tab-completion system in t.p.usage - completion matches are generated dynamically at tab-press time.