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

show fulltimestamp instead of just date in the report #165

Merged
merged 2 commits into from Jan 13, 2018

Conversation

Projects
None yet
5 participants
@meahow

meahow commented Feb 21, 2017

When we generate reports we do it multiple times a day and it's usefull to see what time the rpoert was generated.
Therefore I changed the date field to show the time in addition to the date.

@wolf99

This comment has been minimized.

wolf99 commented Jan 8, 2018

To be completely specific (and pedantic!) should timezone information also be added? For example for the case that people from different time zones are using gcovr on the same project?

@goriy

This comment has been minimized.

Contributor

goriy commented Jan 8, 2018

If you will implement addition of timezone info, please take into consideration possible problems with text encoding (due to some localized names of timezones).
So I suggest to avoid timezone names and I think that it's enough to add just offset from UTC.
Simple way to obtain it is to do something like that:

tztext = "(UTC%+02d:%02d)" % (time.timezone / -(3600), (time.timezone % 3600) / 60)

It results, for example, as: (UTC+2:30)

Note: time.timezone is the offset of the local timezone, in seconds, but with opposite sign (positive to the west, negative to the east).

@latk

This comment has been minimized.

Member

latk commented Jan 8, 2018

Trying write correct software is good 😁 Therefore: if someone can present a real-world use case for including the timezone, the timestamp pattern can be made configurable. Until then, the timezone looks like a YAGNI feature.

The use case for this feature is to distinguish multiple reports from one day. Using a floating time without an explicit zone is entirely sufficient for that. If someone needs to share reports across timezones, as a workaround they can agree on one timezone and set the TZ=UTC environment variable.

I want to merge this after #185 (the HTML tests) since this is an HTML change. However, no changes to the HTML reference files ought to be necessary since the exact date is scrubbed from the file.

@goriy The HTML reports should be in UTF-8. Any encoding problems would be a bug. So if we can provoke a Mojibake, we can fix that bug earlier. That's good.

@latk latk added the needs review label Jan 8, 2018

@goriy

This comment has been minimized.

Contributor

goriy commented Jan 8, 2018

@latk Sorry, but I'm not agree with you in fact that HTML reports should be ONLY in UTF-8.
There is existing source code with strings (comments, string literals, etc) not in UTF-8 for some reasons. In such cases the only sane solution to get proper reports is to make them in the same encoding as source code.

It's not a problem to do so at the moment: text generated by gcovr itself is ascii only, so it remains the same in all 8-bit encodings. Annotated source code goes to html reports untouched, as is. Only necessary thing to get correct report is to set right encoding in headers of report html files. And option --html-encoding allows to set it (yes, it was my patch to add this option 😉 , because such cases are not rare at all for me and to change encoding in all source code of all projects I participate in is an almost impossible task).

If encoding is restricted to UTF-8 only - it's impossible to produce correct reports in those cases without use of external tools either to preprocess files before or to postprocess html reports after. And I think that additional processing here is pointless waste of time.

@latk

This comment has been minimized.

Member

latk commented Jan 9, 2018

@goriy Thank you for pointing that out, I didn't consider source encodings.

@meahow

This comment has been minimized.

meahow commented Jan 9, 2018

Ok. But apart from the source encoding... Is timezone info something anyone needs?

@goriy

This comment has been minimized.

Contributor

goriy commented Jan 9, 2018

For me and my teams it's enough to have local time only. Sharing reports across multiple timezones is definitely not our case at the moment.

Maybe exact timezone information is critical when using such reports for certification purposes (such as DO-178 code coverage analysis reports or something like that), but I'm not sure and I doubt that using only gcov's coverage is enough for that.

@mayeut

This comment has been minimized.

Contributor

mayeut commented Jan 10, 2018

The date was printed out with .isoformat(). Why not do the same with datetime ? It would be a format that's widely accepted IMHO. Unfortunately python 2 does not provide an easy way to get timezone easily so to be 2/3 compatible one would need this:

import datetime
import calendar

def localnow():
    class FixedOffset(datetime.tzinfo):
        def __init__(self, offset):
            self.__offset = datetime.timedelta(minutes = offset)

        def utcoffset(self, dt):
            return self.__offset

        def tzname(self, dt):
            return ''

        def dst(self, dt):
            return datetime.timedelta(0)

    nowutc = datetime.datetime.utcnow().replace(microsecond=0)
    tsutc = calendar.timegm(nowutc.timetuple())
    nowloc = datetime.datetime.fromtimestamp(tsutc)
    tsnow = calendar.timegm(nowloc.timetuple())
    diff = tsnow - tsutc
    assert diff % 60 == 0
    diff = diff / 60
    return nowutc.replace(tzinfo=FixedOffset(0)).astimezone(tz=FixedOffset(diff))

Now if I print this print(localnow().isoformat()), we'll get all we need:

2018-01-10T02:50:55+01:00

The actual timezone is not included but I don't think that matters. All that matters is to get an absolute time. So I get my local time (which is I think most of the users will want) and everyone knows looking at a report that date&time is expressed as UTC+1

@latk latk force-pushed the meahow:master branch from 6fcf905 to af1d320 Jan 13, 2018

@latk

This comment has been minimized.

Member

latk commented Jan 13, 2018

Thank you @meahow for this contribution!

I have rebased this branch on top of master, and updated the HTML tests to account for the new date format.

I will merge this branch as it is, without support for timezones or ISO timestamps – the proposed time format is more human-readable than ISO 8601, e.g. 2018-01-13 14:41:55 vs. 2018-01-13T14:41:55Z.

If someone needs this flexibility, I would welcome a PR that makes the date format configurable, e.g. gcovr --date-format 'strftime format here'.

@latk

latk approved these changes Jan 13, 2018

@latk latk merged commit 17654ae into gcovr:master Jan 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@latk latk added Type: Enhancement and removed needs review labels Jan 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment