-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Implement Tree.pformat_latex_forest #2956
Open
tyomitch
wants to merge
4
commits into
nltk:develop
Choose a base branch
from
tyomitch:develop
base: develop
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
Any alternative suggestions?
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.
Sorry for the delay...
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.
These changes by @tyomitch make
quotes
equivalent toparens
, 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 ofrepr
, 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 tos
. 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.: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.
@tomaarsen thank you for the detailed comment.
My expectation is that pre-existing callers either pass
quotes=True
or don't passquotes
at all; that's why the updated docstring doesn't listFalse
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.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 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 toquotes=("", "")
).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.
@tomaarsen I've added a commit to support
quotes=False