Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

Remove seemingly unnecessary Repeat patterns #201

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

Conversation

bvinc
Copy link
Contributor

@bvinc bvinc commented Oct 4, 2016

This should make the tests in PR #185 much faster.

This was the pattern before my change:

r"""Usage:
  prog a b [-p <param>]...
  prog a c [-p <type> <param>]...
"""
$ program a c -p bool 1 -p bool 0 -p bool 1
{"-p": 3, "<type>": ["bool", "bool", "bool"], "<param>": ["1", "0", "1"]}

Usages:
  Sequence([PatAtom(Command("a")), PatAtom(Command("b")), Repeat(Optional([PatAtom(Short('p')), PatAtom(Positional("param"))]))])
  Sequence([PatAtom(Command("a")), PatAtom(Command("c")), Repeat(Optional([Repeat(PatAtom(Short('p'))), PatAtom(Positional("type")), PatAtom(Positional("param"))]))])

The Repeat(Optional([Repeat(... causes an explosion in the function states. states was being called 1,421,237 times, and producing a vector of 236,975 elements.

This is the pattern after:

Usages:
  Sequence([PatAtom(Command("a")), PatAtom(Command("b")), Repeat(Optional([PatAtom(Short('p')), PatAtom(Positional("param"))]))])
  Sequence([PatAtom(Command("a")), PatAtom(Command("c")), Repeat(Optional([PatAtom(Short('p')), PatAtom(Positional("type")), PatAtom(Positional("param"))]))])

states is being called 7,843 times, and producing a vector of 2,296 elements.

I'm not sure why the removed code was ever written. The concept of "has_repeat" means that the short can possibly repeat under some context somewhere, which seems to have a completely separate meaning from the Repeat(...) pattern. Furthermore, this extra Repeat(...) pattern only affects the second sequence and not the first. That's because after the first sequence is processed, Short('p') is flagged as something that repeats, which affects only the second sequence.

@BurntSushi
Copy link
Member

Hmm, that code was added in a4ee9b to support this feature: docopt/docopt#275

I find it curious that the tests still pass, but I can't yet explain why I added that code in particular. I'll take a closer look when I get a chance.

Thank you for looking into this. :-)

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

Successfully merging this pull request may close these issues.

2 participants