From 4cae8366e0739bb6d74f5522260b6e177636a059 Mon Sep 17 00:00:00 2001
From: Florian Obersteiner <florian.obersteiner@kit.edu>
Date: Wed, 23 Feb 2022 12:34:15 +0100
Subject: [PATCH] comments

---
 src/icartt/dataset.py | 23 +++++++++++++++--------
 tests/test_dataset.py |  2 ++
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/icartt/dataset.py b/src/icartt/dataset.py
index 5c5946e..9504cbc 100644
--- a/src/icartt/dataset.py
+++ b/src/icartt/dataset.py
@@ -362,8 +362,8 @@ class Variable:
         self.miss = miss
 
     def __repr__(self):
-        # TODO: this sould be more meaningful
-        return "ICARTT Variable object repr"
+        # TODO: this sould be something else than __str__ ?
+        return self.desc()
 
     def __str__(self):
         return self.desc()
@@ -419,9 +419,10 @@ class Dataset:
         :return: list of time steps
         :rtype: list
         """
+        # TODO: this method currently does not work at all.
+        #       suggest to also change to return a numpy array of numpy.datetime64
+        #       for consistency with other data output
         return [
-            # TODO see commit f5208db0 - this will do unexpected things if
-            #      self.dateOfCollection is a naive datetime object (think DST transitions...)
             self.dateOfCollection + datetime.timedelta(seconds=x)
             for x in self.independentVariable
         ]
@@ -507,8 +508,11 @@ class Dataset:
         # line 7 - UTC date when data begin, UTC date of data reduction or revision
         # - comma delimited (yyyy, mm, dd, yyyy, mm, dd).
         dmp = f.readline()
-        self.dateOfCollection = datetime.datetime(*map(int, dmp[:3])) # TODO: set UTC ?!
+        self.dateOfCollection = datetime.datetime(*map(int, dmp[:3]))
         self.dateOfRevision = datetime.datetime(*map(int, dmp[3:6]))
+        # TODO: we should either use aware datetime (UTC), date objects or
+        #       numpy.datetime64 here to avoid some 'rough edges' of Python's datetime library...
+
 
         # line 8 - Data Interval (This value describes the time spacing (in seconds)
         # between consecutive data records. It is the (constant) interval between
@@ -865,7 +869,8 @@ class Dataset:
         # TODO: this could be more meaningful
         return "ICARTT Dataset string representation"
 
-    def __init__(self, f=None, loadData=True, splitChar=",", format=Formats.FFI1001): # TODO: why is init comming last?
+    # TODO: why is init comming last?
+    def __init__(self, f=None, loadData=True, splitChar=",", format=Formats.FFI1001):
         """Constructor method"""
         self.format = format
         self.version = None
@@ -883,8 +888,10 @@ class Dataset:
         self.dataSourceDescription = "Musterdatenprodukt"
         self.missionName = "MUSTEREX"
 
-        # TODO: not 100%  sure if this is relevant here, but those dates should refer to UTC
-        # - if you leave the datetime objects naive (no tzinfo set), they represent local time.
+        # TODO: see also comment on _readData method.
+        #       we should either use aware datetime (UTC), date objects or
+        #       numpy.datetime64 here to avoid some 'rough edges' of Python's
+        #       datetime library...
         self.dateOfCollection = datetime.datetime.today()
         self.dateOfRevision = datetime.datetime.today()
         self.dataIntervalCode = [0.0]
diff --git a/tests/test_dataset.py b/tests/test_dataset.py
index d55e8d2..2c78a1a 100644
--- a/tests/test_dataset.py
+++ b/tests/test_dataset.py
@@ -311,6 +311,7 @@ class Create1001TestCase(unittest.TestCase):
 
 
 class BulkIOTestCase(unittest.TestCase):
+    # TODO: need to test invalid input
     # def testInvalid(self):
     #     for fn in fns_fail:
     #         with self.subTest(msg=f"Opening invalid file {str(fn)}"):
@@ -351,6 +352,7 @@ class BulkIOTestCase(unittest.TestCase):
                 ict.write(f=strOut)
                 self.assertTrue(compareFiles(fn, strIn, strOut))
 
+# TODO: should also test dataset methods, e.g. .times
 
 if __name__ == "__main__":  # pragma: no cover
     unittest.main()
-- 
GitLab