Add milliseconds to --log entry timestamps. #6621
Conversation
src/pip/_internal/utils/logging.py
Outdated
| else: | ||
| t = time.strftime(self.default_time_format, ct) | ||
| s = self.default_msec_format % (t, record.msecs) | ||
| return s |
There was a problem hiding this comment.
Hmm, this was the concern I expressed here and that @pfmoore seemed to agree with: #6587 (comment)
For comparison, how simple could the PR look if only Python versions that have the feature out of the box got the enhancement?
There was a problem hiding this comment.
Also, would it solve things just to use the logging module's default datefmt? It looks like that code path uses milliseconds even in Python 2 when I checked.
There was a problem hiding this comment.
Using default_time_format only for Python 3 would require at least 1 check in format and several in the tests. I updated the PR with a simpler approach that still works for both 2 and 3 and removes this block.
There was a problem hiding this comment.
Read my second comment, too. I don’t see a reply to that.
There was a problem hiding this comment.
Sorry, I hadn't refreshed the page when I had posted.
Regarding datefmt, I see it passed to formatTime by the Formatter.format method only if we use %(asctime) in the general formatter format string (1) and if it is passed to formatTime and is truthy then it bypasses the millisecond formatting (2). So I don't think it will work for us here.
There was a problem hiding this comment.
I'm not sure if you understood my suggestion. I was suggesting changing self.formatTime(record, "%Y-%m-%dT%H:%M:%S") to self.formatTime(record) in order to use the logging module's default format.
There was a problem hiding this comment.
Ah, you're right I misunderstood. I prefer the current format since it is standard and if parsing would be easier to split(1) for each line of the log records. When we drop 2 support this can be cleaned up as in the initial revision (with default_time_format) while preserving the same format, but I suspect there will be a lot more refactoring in a lot more places when that happens.
chrahunt
force-pushed
the
feature/log-milliseconds
branch
from
4207255 to
eec53cc
Compare
|
It would be good to add a TODO with “PY2” in the same line with a note
describing what should be done.
…On Sun, Jun 16, 2019 at 4:08 PM Christopher Hunt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/pip/_internal/utils/logging.py
<#6621 (comment)>:
> @@ -118,18 +122,32 @@ def get_message_start(self, formatted, levelno):
return 'ERROR: '
+ if PY2:
+ # Compatibility with default_time_format and default_msec_format from
+ # Python >= 3.3.
+ default_msec_format = '%s,%03d'
+
+ def formatTime(self, record, datefmt=None):
+ ct = self.converter(record.created)
+ if datefmt:
+ s = time.strftime(datefmt, ct)
+ else:
+ t = time.strftime(self.default_time_format, ct)
+ s = self.default_msec_format % (t, record.msecs)
+ return s
Ah, you're right I misunderstood. I prefer the current format since it is
standard and if parsing would be easier to split(1) for each line of the
log records. When we drop 2 support this can be cleaned up as in the
initial revision (with default_time_format) while preserving the same
format, but I suspect there will be a lot more refactoring in a lot more
places when that happens.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#6621?email_source=notifications&email_token=AACW33VJART2FIPD4WYKSFTP23BX5A5CNFSM4HYRZMF2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3VI7BA#discussion_r294104561>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACW33SINQPLZMURVQVNYKLP23BX5ANCNFSM4HYRZMFQ>
.
|
|
TODO added. |
lock
bot
added
the
auto-locked
Resolves #6587.