ANSI rendering of rst markup. #24
Conversation
|
Rebased. |
| @@ -5,4 +5,4 @@ universal=1 | |||
| source=defopt | |||
|
|
|||
| [coverage:report] | |||
| fail_under=98 | |||
| fail_under=97 | |||
There was a problem hiding this comment.
Why did you need to change this? I don't think your changes reduced coverage.
There was a problem hiding this comment.
The return in
if not isinstance(tag, str) and tag is not None:
return
doesn't get exercised (and I don't think it's worth writing a test to specifically test it -- it comes straight from the upstream implementation). That's enough for dropping the coverage.
| @@ -19,12 +19,13 @@ | |||
| install_requires=[ | |||
| 'docutils', | |||
| 'sphinxcontrib-napoleon>=0.5.1', | |||
| 'funcsigs;python_version<"3"', | |||
| 'funcsigs;python_version<"3.3"', | |||
There was a problem hiding this comment.
For the record, I don't intend to support 3.0-3.2, but I don't have a problem with changing this.
There was a problem hiding this comment.
I don't plan to use them either, but that's just a more convenient way to say "function signatures were introduced in 3.3"
defopt.py
Outdated
| def _itertext_ansi(node): | ||
| # Implementation modified from `ElementTree.Element.itertext`. | ||
| tag = node.tag | ||
| if not isinstance(tag, str) and tag is not None: |
There was a problem hiding this comment.
This is basestring in the Python 2 version of itertext. We should account for that.
defopt.py
Outdated
| except ImportError: # pragma: no cover | ||
| pass | ||
| else: | ||
| colorama.init() # pragma: no cover |
There was a problem hiding this comment.
I know this is the way colorama tells you to use it, but since defopt promises to not modify the functions given, I think it's also reasonable to expect it to have no side effects (especially when you're not even using the CLI). Instead, I think we should use colorama.colorama_text to wrap just the call to parser.parse_args inside run.
defopt.py
Outdated
| tag = node.tag | ||
| if not isinstance(tag, str) and tag is not None: | ||
| return | ||
| ansi_code = {"emphasis": "\033[1m", "strong": "\033[3m"}.get(tag, "") |
There was a problem hiding this comment.
You have these reversed - 1 is bold, 3 is italic.
|
Thanks for your work. Overall the implementation is great, there are just a few things I'd like to get cleaned up before I merge this in. |
defopt.py
Outdated
| def _itertext_ansi(node): | ||
| # Implementation modified from `ElementTree.Element.itertext`. | ||
| tag = node.tag | ||
| if (not isinstance(tag, (basestring if sys.version_info < (3,) else str)) |
There was a problem hiding this comment.
Sorry to keep getting you to change this, but this is a bit cluttered for my taste. Can you instead add a definition for basestring at the top with the rest of the compatibility stuff?
defopt.py
Outdated
| @@ -12,6 +12,7 @@ | |||
| from argparse import (SUPPRESS, ArgumentParser, RawTextHelpFormatter, | |||
| ArgumentDefaultsHelpFormatter) | |||
| from collections import defaultdict, namedtuple, OrderedDict | |||
| import contextlib | |||
There was a problem hiding this comment.
These are split into separate subgroups for import x and from x import y. I was aiming for what you get from isort, though I'm not actually running the tool so this may differ slightly.
colorama catches the raw ANSI **strong** code and handles it properly on Windows, but does not expose *emphasis* (because the Windows console cannot render it -- it will remove still remove it from the stream though, so there isn't any garbled output). So we directly use raw ANSI codes.
|
done |
|
Perfect, thanks! |
See #22. Note that this PR depends on #20 (the first few commits are from that PR).