-
Notifications
You must be signed in to change notification settings - Fork 314
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
Fix issue #93 #96
Fix issue #93 #96
Conversation
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.
Thanks @merajhashemi!
I made a small suggestion to generalize the matching.
@@ -212,7 +212,7 @@ def _remove_iffalse_block(text): | |||
|
|||
def _remove_comments_inline(text): | |||
"""Removes the comments from the string 'text' and ignores % inside \\url{}.""" | |||
if 'auto-ignore' in text: | |||
if '%auto-ignore' in text.split(): |
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 would generalize this a bit more to accept spaces in between the %
and auto-ignore
. It can be something like if text.replace(' ', '').startswith('%auto-ignore')
.
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.
@jponttuset I didn't allow spaces between '%' and 'auto-ignore' because the arXiv documentation doesn't clarify if it's allowed. And the examples I saw all followed this format.
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'd still be a bit more lenient here and allow spaces. One problem I now see, though, is that this keeps the whole line, and you might have a super-secret message right after auto-ignore :)
So maybe I'd do:
if text.replace(' ', '').startswith('%auto-ignore'):
return '%auto-ignore'
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.
We should also consider the possibility of %auto-ignore being in the middle of a line. The new commit I just pushed takes this into account as well.
This PR fixes #93. Two new test were added to ensure
auto-ignore
outside a comment tag is treated as normal text.