Sage-Bionetworks / synapsePythonClient

Programmatic interface to Synapse services for Python
https://www.synapse.org
Apache License 2.0
67 stars 68 forks source link

[SYNPY-1358] Correction of timestamp in annotations from manifest file #1020

Closed BryanFauble closed 10 months ago

BryanFauble commented 10 months ago

Problem:

  1. When a timestamp is given in a manifest.tsv file the current code is storing the data as a string annotation instead of a date.
  2. When a datetime or date object is created we did not account for the timezone of the current machine or what is defined in the datetime object.
  3. When the UTC timestamp was coming back from the synapse API the code was always converting the timestamp into local time, however, UTC time is what synapse is giving us.

Solution:

  1. I added logic when creating timestamps from the manifest.tsv file to check the format of the string. If it matches a date format we are going to use that type in order to create the annotation.
  2. I am taking into account the timezone of the current machine or, if defined, the timezone defined on the datetime object.
  3. For data coming back from the synapse server we are now taking it in as UTC time which allows anyone who needs to use it have the correct time. This also allows for the timestamp to correct print in the manifest.tsv

Testing:

  1. I manually tested with several annotations. For example given this manifest file:
    path    parent  name    id      synapseStore    contentType     used    executed        activityName    activityDescription     my_key_timestamp_date_yesterday my_key_long     my_key_bool     my_key_string   my_key_double   my_key_timestamp_datetime       my_key_timestamp_date
    /home/bfauble/annotationTesting/file.txt        syn52919599     file.txt        syn53085003     True    text/plain                                      2023-12-04T07:00:00Z    1       False   b       1.2     2023-12-05 23:37:02.995000+00:00        2023-12-05 07:00:00+00:00

    image

  2. I update the unit and integration tests to allow for this logic.
pep8speaks commented 10 months ago

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1062:89: E501 line too long (141 > 88 characters) Line 1063:89: E501 line too long (121 > 88 characters) Line 1064:89: E501 line too long (141 > 88 characters) Line 1065:89: E501 line too long (141 > 88 characters) Line 1066:89: E501 line too long (141 > 88 characters) Line 1067:89: E501 line too long (141 > 88 characters) Line 1068:89: E501 line too long (141 > 88 characters)

Line 4:10: E401 multiple imports on one line

Line 37:89: E501 line too long (125 > 88 characters) Line 39:89: E501 line too long (162 > 88 characters) Line 166:89: E501 line too long (94 > 88 characters)

Line 7:10: E401 multiple imports on one line

Comment last updated at 2023-12-06 16:07:21 UTC