Skip to content
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

[BUGFIX] contrib: yanic-import-timestamp for latest hopglass timestamp format #67

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

mweinelt
Copy link
Contributor

No description provided.

@genofire
Copy link
Member

Auch wenn keiner mehr version 1 nutzt, sollte dort ebenfalls eine importierung auf Zeitzonen stand finden.
Ggf. auch wirklich eine Überprüfung, ob und in welchen Zeitzonenformat es sich handelt.

@mweinelt
Copy link
Contributor Author

ffmap-backend nutzt datetime.datetime.now().isoformat()

>>> import datetime
>>> datetime.datetime.now().isoformat()
'2017-05-31T18:30:19.759610'

Das dürfte doch Version 1 sein, oder?

@@ -35,7 +35,7 @@ if oldnodedb['version'] == 2:
for nodeid, node in newnodedb['nodes'].items():
for oldnode in oldnodedb['nodes']:
if oldnode['nodeinfo']['node_id'] == nodeid:
node['firstseen'] = "{}+0100".format(oldnode['firstseen'])
node['firstseen'] = "{}+0100".format(oldnode['firstseen'][:-5])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? Always remove the last 5 bytes? I think a regexp replacement is better in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's not a sufficient solution for a lazy migration script I'd rather use datetime.strptime.

@genofire genofire added the wip label Jun 1, 2017
@genofire genofire changed the title contrib: update yanic-import-timestamp for latest hopglass timestamp format [BUGFIX] contrib: yanic-import-timestamp for latest hopglass timestamp format Jun 1, 2017
@mweinelt mweinelt force-pushed the master branch 2 times, most recently from 107926d to 866f89e Compare June 2, 2017 22:02
@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage remained the same at 57.457% when pulling 866f89e on mweinelt:master into 88975d2 on FreifunkBremen:master.

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage increased (+0.2%) to 57.688% when pulling 866f89e on mweinelt:master into 88975d2 on FreifunkBremen:master.

Copy link
Member

@genofire genofire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better but still not perfect.
(Sry)

dt = datetime.strptime(oldnode['firstseen'], v2_date_format)
dts = dt.strftime(yanic_date_format)
if node['firstseen'] != dts:
node['firstseen'] = dt.strftime(yanic_date_format)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warum nicht dts wie in version 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unvollständig refactored …

for nodeid, node in newnodedb['nodes'].items():
for oldnode in oldnodedb['nodes']:
if oldnode['nodeinfo']['node_id'] == nodeid:
node['firstseen'] = "{}+0100".format(oldnode['firstseen'])
count+=1
dt = datetime.strptime(oldnode['firstseen'], v2_date_format)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not be so easy 😢
The ffmap-backend could also generate v2.
See here: https://github.com/ffnord/ffmap-backend/blob/respondd/lib/nodes.py#L53-L55
We need a try, if it is a timestamp with timezone.

Maybe something like if 'Z' in old and if '+' in old else with v1 form
and a try at the end with dt is over 0 and error handling (maybe a function for both v1 and v2)

Also some additional refactoring and more verbosity.
@coveralls
Copy link

coveralls commented Jun 3, 2017

Coverage Status

Coverage increased (+0.2%) to 57.688% when pulling 09f8bd5 on mweinelt:master into 88975d2 on FreifunkBremen:master.

Copy link
Member

@genofire genofire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bin mir nocht ganz sicher, ob bei v1_datezone_format nicht lieber doch die Zeitzone +1 nicht +0 genommen werden sollte. Doch ein Stunde Abweichung inzweifelsfall fällt nun wirklich nicht ins Gewicht.
Vielen Dank für die Überarbeitung.

@mweinelt
Copy link
Contributor Author

mweinelt commented Jun 3, 2017

https://docs.python.org/3/library/datetime.html#datetime.datetime.now

datetime.now() enthält per default keine tzinfo und ist daher naiv, also UTC. Daher ist die Zeitzone +0000, nicht +0100 (CET) oder gar +0200 (CEST).

@genofire genofire added review and removed wip labels Jun 12, 2017
@genofire genofire merged commit de96847 into FreifunkBremen:master Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants