Implement Tree.pformat_latex_forest

[tyomitch]

Includes some refactoring to Tree.pformat

[stevenbird]

To the best of my knowledge we've avoided this kind of polymorphism in NLTK. I think it needs some consideration if we're going to start doing this.

[tyomitch]

To the best of my knowledge we've avoided this kind of polymorphism in NLTK. I think it needs some consideration if we're going to start doing this.

Any alternative suggestions?

[stevenbird]

Sorry for the delay...

[tomaarsen]

These changes by @tyomitch make quotes equivalent to parens, which is a logical move. Then, beyond that, he also allows backwards compatibility so True and False can still be used. I don't necessarily see an issue with that, especially because using True means the use of repr, which cleverly uses the right quotes depending on the string.

That said, at a glance the implementation is likely bugged. On line 841 - what if the child is a string, and quotes = False. Then, the if-statement is true, and "\n" + indent_str + quotes[0] + str(child) + quotes[1] is added to s. However, quotes[0] will throw an exception.

An alternative implementation that avoids the polymorphism simply reverts quotes to what it was, and adds e.g. quote_strings=None. The last else branch is where the quotes should be added, so then we can do e.g.:

            elif isinstance(child, str) and not quotes:
                s += "\n" + " " * (indent + 2) + "%s" % child
            else:
                if not quote_strings:
                    s += "\n" + " " * (indent + 2) + repr(child)
                else:
                    s += "\n" + " " * (indent + 2) + quote_strings[0] + str(child) + quote_strings[1]

[tyomitch]

@tomaarsen thank you for the detailed comment.

My expectation is that pre-existing callers either pass quotes=True or don't pass quotes at all; that's why the updated docstring doesn't list False as an acceptable input.

With the code you suggest, when quotes is False, quote_strings get ignored, which may be confusing to the user. To apply custom quote characters, he'd need to pass both parameters, which is less user-friendly. I have no problem with that -- just flagging it up.

[tomaarsen]

I agree that that would be confusing. I don't dislike your current implementation per se, although I would probably want to see support for quotes=False (i.e. have it be equivalent to quotes=("", "")).

[tyomitch]

@tomaarsen I've added a commit to support quotes=False