Formatter supports escape characters #688
Conversation
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 1563 1565 +2
Branches 255 256 +1
=====================================
+ Hits 1563 1565 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey @remeika we just have a few questions for you. We've been playing around with the new patterns at https://regex101.com/r/BTD1Gk/1/ and aren't sure why the last character in every escaped bracket is matching separately.
arrow/formatter.py
Outdated
| @@ -12,8 +12,9 @@ | |||
| class DateTimeFormatter(object): | |||
|
|
|||
| _FORMAT_RE = re.compile( | |||
| r"(YYY?Y?|MM?M?M?|Do|DD?D?D?|d?dd?d?|HH?|hh?|mm?|ss?|SS?S?S?S?S?|ZZ?Z?|a|A|X)" | |||
| r"(\[(?:(?=(?P<literal>\\\\\[|\\\\\]|[^]]))(?P=literal))*\]|YYY?Y?|MM?M?M?|Do|DD?D?D?|d?dd?d?|HH?|hh?|mm?|ss?|SS?S?S?S?S?|ZZ?Z?|a|A|X)" | |||
There was a problem hiding this comment.
Can you walk us through this new pattern?
There was a problem hiding this comment.
In particular, what is the purpose of the two pairs of escaped slashes?
arrow/formatter.py
Outdated
| ) | ||
| _ESCAPED_BRACKET_RE = re.compile(r"\\\\(\[|\])") |
arrow/formatter.py
Outdated
| @@ -25,6 +26,11 @@ def format(cls, dt, fmt): | |||
|
|
|||
| def _format_token(self, dt, token): | |||
|
|
|||
| if token and token.startswith("[") and token.endswith("]"): | |||
| return self._ESCAPED_BRACKET_RE.sub( | |||
| lambda m: m.group(1), token[1:-1] | |||
There was a problem hiding this comment.
Could you explain exactly what the lambda function is doing here? Maybe adding a few comments to the source code would help.
|
Hey @remeika, any update on this? We'd love to merge this in, but we have a few clarifying questions. |
|
@jadchaar Hey let me review the comments and reply later tonight! |
remeika
force-pushed
the
feature/formatter-escape-chars
branch
from
7b55e72
to
5c84035
Compare
|
Sorry for the late response here @jadchaar @systemcatch ! I've removed all handling of escaped brackets, which I think was motivated by this comment by @bsidhom; as I said, this basic implementation was his. But I don't think handling escaped brackets is really necessary, so this feature now does not handle them at all, which really simplifies things. I think this code is now ready to go! |
|
@systemcatch As to why the last character of every escaped token is matching: I think this is an artifact of the workaround I'm using to emulate atomic capture groups. See the comment I added for details. |
The
DateTimeFormatternow supports brackets as escape characters. Based almost entirely off the work of @bsidhom.One note: I could not get coverage to pass on of my lines of code, and had to add a no cover pragma. If anyone has an idea why code coverage is unhappy with
formatter.py:31, I'd love to fix it.Fixes #35